Two-level workspace estimation for MatMulNBits (CUDA EP, fpA_intB/CUTLASS) - #29811
Ti-Tai Wang (titaiwangms) merged 13 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds a two-level workspace estimation path for CUDA MatMulNBits (fpA_intB/CUTLASS) so workspace bytes can be computed deterministically (1) at partition time from node metadata and (2) at kernel-instance time from shapes, using a shared overflow-checked formula.
Changes:
- Extract fpA_intB GEMM workspace sizing into a shared
ComputeFpAIntBGemmWorkspaceSize()helper and route the CUTLASS runner through it. - Centralize fpA_intB eligibility + “effective SM for workspace” logic and reuse it from both the
MatMulNBitsconstructor and the Level-1 estimator. - Introduce a new
OpKernel::DeclareWorkspaceRequirements()default no-op (mirrored in the plugin adapter hierarchy) plus a lightweightWorkspaceRequirementPOD; add unit tests for the pure helpers.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/test/providers/cuda/test_cases/matmul_nbits_workspace_test.cu | Adds host-only unit tests for eligibility, effective-SM mapping, and the shared workspace formula. |
| onnxruntime/core/providers/cuda/cuda_execution_provider.cc | Logs a Level-1 MatMulNBits workspace estimate during GetCapability() (pilot, log-only). |
| onnxruntime/contrib_ops/cuda/quantization/matmul_nbits.h | Declares shared eligibility + Level-1 estimator API and wires constructor through the shared decision. |
| onnxruntime/contrib_ops/cuda/quantization/matmul_nbits.cc | Implements shared eligibility, Level-1 estimator, and Level-2 DeclareWorkspaceRequirements() override. |
| onnxruntime/contrib_ops/cuda/quantization/matmul_nbits_workspace_estimate.h | Adds a slim forward-decl header for calling the Level-1 estimator from core CUDA EP code. |
| onnxruntime/contrib_ops/cuda/llm/fpA_intB_gemm/fpA_intB_gemm.h | Adds the shared overflow-checked ComputeFpAIntBGemmWorkspaceSize() helper. |
| onnxruntime/contrib_ops/cuda/llm/fpA_intB_gemm/fpA_intB_gemm_template.h | Makes getWorkspaceSize() delegate to the shared helper. |
| include/onnxruntime/ep/adapter/op_kernel.h | Mirrors the new DeclareWorkspaceRequirements() virtual default no-op for the plugin adapter hierarchy. |
| include/onnxruntime/core/framework/workspace_requirement.h | Introduces a lightweight WorkspaceRequirement POD shared across both OpKernel hierarchies. |
| include/onnxruntime/core/framework/op_kernel.h | Adds the new DeclareWorkspaceRequirements() virtual default no-op on the in-tree OpKernel. |
|
Ran this PR through a 5-pass review (readability / correctness / adversarial / spec-adherence / cross-module integration). No Critical findings — the core design (shared That said, there are a few things worth addressing before merge: Major
Minor
Happy to take another pass once these are addressed, especially #1 — that's the one that would let this PR demonstrate its own value empirically rather than by static argument. |
…o-level estimation Major 1: Add MatMulNBitsWorkspace.EndToEndWorkspaceAgreement, a GPU-requiring test that builds a single-node MatMulNBits model (fp16/int4/block_size=32, N=256/K=1024), runs it through a real InferenceSession on the CUDA EP, and asserts that the partition-time estimate (EstimateMatMulNBitsWorkspace), the kernel-instance estimate (DeclareWorkspaceRequirements) and the actual runtime request (LastComputeWorkspaceBytes) are all exactly equal (proven: 1792 == 1792 == 1792). The test lives in a new core-world .cc bridged to the provider world by a slim probe header, so a full session and provider internals can coexist. Also exports onnxruntime_provider_test symbols (ENABLE_EXPORTS) so the dlopen'd cuda_ut module can resolve InferenceSession at load time. Major 2: Document the known Level-1/constructor eligibility divergence (session-config ep.cuda.fpa_intb_gemm vs env var). Fully fixing it needs ConfigOptions threaded through IExecutionProvider::GetCapability(), a cross-EP interface change; Level 1 is log-only so the divergence is safe. Documented in EstimateMatMulNBitsWorkspace. Major 3: FpAIntBPackingSmForKernel() now delegates to the shared EffectiveFpAIntBWorkspaceSm(), so the compiler (not a comment) guarantees Level 1 and Level 2/runtime resolve the effective arch identically. Minor 4: Fix build-macro guards from defined(USE_FPA_INTB_GEMM) to USE_FPA_INTB_GEMM (value check) in matmul_nbits_workspace_estimate.h and cuda_execution_provider.cc. Minor 5: Document LastComputeWorkspaceBytes() staleness. Minor 6: Remove unused FpAIntBEligibility struct; CheckFpAIntBEligibility returns bool. Minor 7: Remove dead block_size != 0 conjunct. Minor 8: Update test file-header comments. Also fixes a pre-existing build blocker: EstimateMatMulNBitsWorkspace's get_attr lambda used NodeAttributes::find()/it->second, which the provider-bridge IteratorHolder does not support; switched to range iteration. Renamed the pure-function test from .cu to .cc (it has no device code) to avoid nvcc choking on gtest's re2-backed regex support.
…precise E2E comment - cmake: WIN32-gate link onnxruntime_providers_cuda_ut against the onnxruntime_provider_test import lib so InferenceSession symbols resolve at link time on Windows (MODULE libs can't have unresolved externals). - cmake: add matmul_nbits_workspace_test.cc and matmul_nbits_e2e_workspace_test.cc to the plugin-mode explicit test list so they run in BUILD_CUDA_EP_AS_PLUGIN CI. - test: clarify e2e comment/scope that Level 1 calls the estimator directly, not a full GetCapability()-driven partition-time run.
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
The shared workspace formula, overflow handling, effective-SM routing, and current-head macro guards look consistent for the in-tree CUDA EP. I am requesting changes for the plugin test integration: the newly added tests depend on the concrete in-tree MatMulNBits kernel and cannot safely run through the plugin wrapper as currently implemented.
…der_api.h convention MSVC treats the struct/class keyword mismatch as warning C4099, which is escalated to a hard error (C2220) under the CUDA EP's /W4 all-warnings-as-error build flags on Windows. onnxruntime::Node is forward-declared as 'struct' in core/providers/shared_library/provider_api.h; this header used 'class', causing Windows GPU CUDA CI and Windows GPU Kernel Documentation Validation to fail while Linux builds (used for all local GPU verification) were unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ade1ab12-1137-46bb-a92b-e730a6f28bf4
matmul_nbits_workspace_test.cc and matmul_nbits_e2e_workspace_test.cc directly construct the concrete in-tree CUDAExecutionProvider / MatMulNBits<T> kernel types. In a plugin build (BUILD_CUDA_EP_AS_PLUGIN), the provider lives behind the plugin DLL and SessionState::GetKernel() returns a core PluginEpOpKernel whose DeclareWorkspaceRequirements() is the default no-op (the MatMulNBits override is compiled out). Downcasting that wrapper via GetMatMulNBitsLastComputeWorkspaceBytes()'s static_cast is undefined behavior, and depending on linkage this can instead fail earlier on unresolved provider-internal symbols. This reverts the round-2 fix's addition of these sources to onnxruntime_test_providers_cuda_plugin_internal_test_src, per #29811 review feedback. Re-adding them requires first bridging DeclareWorkspaceRequirements through OrtKernelImpl for plugin kernels (tracked as Phase B / future follow-up work), not exercising the in-tree concrete-kernel downcast against a plugin session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ade1ab12-1137-46bb-a92b-e730a6f28bf4
Implements the three remaining Test-plan cases from issue #29810 (Tests A/B were already covered by EndToEndWorkspaceAgreement). All are additive, test-only changes in matmul_nbits_e2e_workspace_test.cc; no production code changed. Test C - FixedShapeViaFreeDimensionOverride: input A declared ["seq", K]; SessionOptions binds free dim "seq"->512 before Initialize(). Proves the fixed override value flows through the NodeArg shape into Level 1 (estimator) and Level 2 (DeclareWorkspaceRequirements) and matches the real runtime request (all == 3584 bytes for M=512). Also asserts requirements[0].slot_id == 0. Test D - DynamicShapeNoOverrideFallsBack: input A declared ["seq", K] with no override. Level 1 returns nullopt (symbolic leading dim), Level 2 returns empty requirements (unknown dim -> fallback), and Run() still succeeds via the live GetScratchBuffer path (runtime request 1792 bytes). Test E - NonMatMulNBitsKernelDeclaresNoWorkspace: a control Add kernel (which does not override DeclareWorkspaceRequirements) hits the OpKernel base-class no-op -> Status::OK() and an empty requirements vector (verified it clears a pre-populated vector rather than appending). Tests C/D GTEST_SKIP on GPUs with compute capability < 7.5 (matching the production CheckFpAIntBEligibility gate) so they stay portable across CI GPUs, and derive the Level-2 TensorShape from the actual (possibly-overridden) NodeArg proto via the production onnxruntime::utils::GetTensorShapeFromTensorShapeProto converter, exercising the real TensorShapeProto -> TensorShape path (including dim_param -> -1) rather than a hand-built stand-in. Refactors BuildMatMulNBitsModelBytes to optionally declare A's leading dim as a symbolic dim_param (for Tests C/D) and adds a tiny fp32 Add model builder (Test E).
…2E test Round-4 review fixes on top of the MatMulNBits two-level workspace-estimation change. Issue 1 (Windows/MSVC C4099, GCC/Clang -Wmismatched-tags): the previous 'class Node' -> 'struct Node' change in matmul_nbits_workspace_estimate.h only relocated the tag mismatch. The header has exactly two includers living in different Node 'worlds': cuda_execution_provider.cc (provider_api.h: 'struct Node') and matmul_nbits_e2e_workspace_test.cc (core/graph/graph.h: 'class Node'). Any single tag we pick mismatches one of them. Both includers already declare Node (via their own core/bridge headers) BEFORE including this header, so we now omit the forward declaration entirely and rely on the includer's declaration, with a comment explaining why. Verified include order: provider_api.h precedes the header in the EP TU; core/graph/graph.h precedes it in the test TU. Issue 2 (portability): the pre-existing EndToEndWorkspaceAgreement (Test A/B) only checked cudaGetDeviceCount() > 0, so on a real GPU with compute capability < 7.5 its ASSERT_TRUE(level1.has_value()) would FAIL rather than skip (production CheckFpAIntBEligibility rejects fpA_intB below SM 7.5). Applied the same CudaDeviceComputeCapabilityOrNegative() + kMinFpAIntBSm >= 75 GTEST_SKIP guard already used by Tests C/D. Minor cleanups: extracted FindNodeByOpType(graph, op_type) helper to replace the four duplicated node-search loops, and documented why the Add control test (Test E) needs only a device-presence check (no SM>=7.5 requirement).
…nit-test TU with DISABLE_CONTRIB_OPS - matmul_nbits_e2e_workspace_test.cc: removed accidentally duplicated header comment block (lines were repeated verbatim). - matmul_nbits_workspace_test.cc: this TU is globbed into onnxruntime_providers_cuda_ut unconditionally, but includes matmul_nbits.h (a contrib op header) and references MatMulNBits<T> symbols that don't exist when DISABLE_CONTRIB_OPS is set, since contrib_ops sources are excluded from the build in that configuration -> would cause link failures. Guard the file the same way as its companion e2e test (!defined(DISABLE_CONTRIB_OPS) && USE_FPA_INTB_GEMM), and update the trailing #endif comment to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… CI check Unrelated to this PR's MatMulNBits workspace-estimation work; the branch was 23 commits behind main and docs/ContribOperators.md / docs/OperatorKernels.md had drifted out of sync with main's QMoE nvfp4 support, causing the 'Windows GPU Kernel Documentation Validation' CI check to fail.
There was a problem hiding this comment.
Re-reviewed at head 5e4d471. The two-level workspace-estimation pilot is in good shape and my earlier blocking concern is resolved.
Verified this round:
- Single-source-of-truth refactor is behavior-preserving.
CheckFpAIntBEligibilityreproduces the old constructor gate: the new fp16/bf16 dtype gate is a no-op inside theif constexpr (MLFloat16||BFloat16)block, and the newweight_prepacked/SM90 validations are unreachable in the constructor becauseORT_ENFORCE(weight_prepacked_ in {0,1,2})andValidateSm90PrepackedWeightSupport(sm_, block_size_)already throw earlier for those cases.kInputIndexGroupIndex == 4matches on both paths. - Level-2 vs runtime arch consistency is correct.
getWorkspaceSizereads the runner'ssm_, whichsetArch(FpAIntBPackingSmForKernel())already overwrote to the effective arch duringInitGemmProfiler, so it agrees with the Level-2effective_smeven on an SM90 device running the SM80-compat layout. - Overflow handling is sound. ORT's
SafeIntusesSafeIntExceptionHandler<OnnxRuntimeException>(ORT_THROW), so thecatch (const OnnxRuntimeException&)inComputeFpAIntBGemmWorkspaceSize/EstimateMatMulNBitsWorkspace/DeclareWorkspaceRequirementscorrectly catches overflow and narrowing failures and degrades tonullopt/dynamic-fallback. The new SM90SafeInt<size_t>path also fixes a latent pre-castintoverflow in the old formula. - Cmake fix confirmed: the workspace tests are now excluded from the plugin-internal list with an explanatory comment; resolving that thread.
Dismiss request change since there is no blocking issue in latest commit.
…ignment field - last_compute_workspace_bytes_ (test-only instrumentation) now uses memory_order_relaxed for both store and load, since the pilot's tests are single-threaded and don't need seq_cst ordering. - Add an optional std::optional<size_t> alignment field to WorkspaceRequirement, defaulting to nullopt. No kernel sets it today (CUDA's allocator already guarantees >= 256-byte alignment, sufficient for MatMulNBits' and GroupQueryAttention's/Attention's workspace buffers), but it reserves the field now to avoid an ABI-breaking change later if a future kernel needs stricter alignment or a shared-arena packer needs to co-locate multiple kernels' slots. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ade1ab12-1137-46bb-a92b-e730a6f28bf4
- cuda_kernel_workspace_inventory.md: reclassify MatMulNBits/fpA_intB_GEMM static determinability from upper-bound-only to exact, confirmed by this pilot's closed-form workspace formula (m, n, k, sm, multiProcessorCount). MOE is left unchanged (different runner, not investigated here). - future_directions_constrained_env.md: sync the WorkspaceRequirement struct with the new alignment field; document that neither Level 1 nor Level 2 yet receives real runtime shapes (only this pilot's test harness calls them today); document the confirmed math-helper vs. graph-parsing reuse boundary; enumerate the remaining concrete cost of a real plugin-side override (answers reviewer question about the plugin EP plan). - New skill workspace-estimation-shared-header: captures the Node forward-declaration cross-DLL-world pitfall and the shared-header graph-type-free rule for future agents working on this boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ade1ab12-1137-46bb-a92b-e730a6f28bf4
…lity - WorkspaceRequirement.alignment: std::optional<size_t> -> size_t alignment_bytes (0 = allocator default). critical-reviewer flagged that std::optional's layout is not guaranteed stable across compilers/STL versions, which undermines the struct's own stated goal of being usable across a future plugin-DLL boundary. A scalar with a sentinel value avoids that risk. Updated both construction sites and rebuilt/retested (CUDA_EP_Unittest.All: 58 pass / 2 unrelated skip / 0 fail). - cuda_kernel_workspace_inventory.md / future_directions_constrained_env.md / workspace-estimation-shared-header skill: applied readability-reviewer's findings — moved long table-cell caveats to footnotes, shortened the alignment_bytes inline code comment (moved rationale to prose), removed a stray double blank line, de-duplicated a takeaway bullet, added a matching "how to confirm" step to the skill's second pitfall, and added MatMulNBits/fpA_intB to the multiProcessorCount row in the API-requirements table (previously missing despite being covered in the text above). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ade1ab12-1137-46bb-a92b-e730a6f28bf4
Description
Pilot for Phase A of the memory roadmap (#29775): give the CUDA EP a way to know a
MatMulNBitsnode's transient CUTLASS scratch ("workspace") size beforeRun(), instead of relying on the accountant's blanket 1.5x heuristic that never sees the workspace term. Scoped to one kernel, in-tree only.The workspace formula is extracted into one shared, stateless helper that all three call sites funnel through, so the numbers are identical by construction:
Shared formula (
fpA_intB_gemm.h):ComputeFpAIntBGemmWorkspaceSize(m, n, k, sm, multi_processor_count)— pure arithmetic, no device/instance state. Overflow-checked end-to-end withSafeInt<size_t>(both SM90 and non-SM90 branches, including ceil-div numerators); returnsnullopton overflow rather than throwing. The 5 tile constants are promotedprotected→publicso the free function can read them.CutlassFpAIntBGemmRunner::getWorkspaceSizenow delegates to it — byte-for-byte identical on valid inputs.Eligibility single-source-of-truth (
matmul_nbits.{h,cc}):CheckFpAIntBEligibility+EffectiveFpAIntBWorkspaceSmcentralize the multi-part fpA_intB path decision (dtype, option/env gate, nbits/block/alignment, SM, prepacked/SM90 constraints). The constructor is refactored to call it (behavior-preserving), so Level 1 can never disagree with the runtime path.Level 1 — partition-time (
EstimateMatMulNBitsWorkspace, wired intoCUDAExecutionProvider::GetCapability()): a plain op-type-dispatched free function (no virtual, no registration) that estimates from node attributes alone, before any kernel instance exists. Exposed via a slim forward-declaring header; log-only for this pilot (does not alter the accountant's budget number). Both the call site and the#includeare double-guarded with#if !defined(DISABLE_CONTRIB_OPS) && defined(USE_FPA_INTB_GEMM).Level 2 — instance-level (
MatMulNBits::DeclareWorkspaceRequirements): new default no-opvirtualonOpKernel. Becausematmul_nbits.cccompiles into two hierarchies, the no-op is mirrored on both the in-tree and the plugin adapterOpKernel(following thePrePackprecedent); the real CUTLASS-backed override is in-tree only (#ifndef BUILD_CUDA_EP_AS_PLUGIN). It feeds the same effective arch the runner resolves aftersetArch()(FpAIntBPackingSmForKernel(), not rawsm_), so it equals the runtime request when the queried shape matches.Boundary-safe struct:
WorkspaceRequirement(size_bytes,slot_id) lives in a new lightweightcore/framework/workspace_requirement.hso bothOpKernelhierarchies can include it without the adapter pulling incore/framework/op_kernel.h. This pilot uses one slot; theInlinedVector/slot_idgenerality is reserved for later multi-buffer kernels (e.g. Attention).Unit tests cover the pure helpers: formula exact values (non-SM90 + SM90), overflow/negative-dim
nullopt,EffectiveFpAIntBWorkspaceSm(drift guard A), andCheckFpAIntBEligibility(drift guard C).Motivation and Context
The workspace size is currently computed live inside
ComputeInternal()(getWorkspaceSize(m, n, k)), so the partition-time budget accountant has zero visibility into it and falls back to a 1.5x multiplier oninitializer + outputbytes — which both under- and over-estimates, since the CUTLASS scratch is fp32 and unrelated to output size.MatMulNBitswas chosen as the pilot because its formula is real, non-trivial, kernel-instance-independent, and single-path, and the op dominates quantized-LLM projection cost.Notes for reviewers:
virtualto in-treeOpKernelchanges vtable layout — a known consideration for out-of-tree EPs compiled against older headers; the pilot does not solve it but flags it so it isn't discovered silently.