fix(hip): unify result shapes and accept dynamic batched MatMul - #581
Draft
fhanuman wants to merge 10 commits into
Draft
fix(hip): unify result shapes and accept dynamic batched MatMul#581fhanuman wants to merge 10 commits into
fhanuman wants to merge 10 commits into
Conversation
Share broadcast, Gemm, and MatMul extent logic so converters allocate the same shapes exposed downstream, while preserving cached MatMul ABI compatibility and rejecting unsupported partial batch broadcasts. Co-Authored-By: GPT-5.6 Sol <noreply@openai.com> Made-with: Cursor
Emit explicit errors for invalid strided-batch shapes and document why repeated broadcast operands reuse their first mixed shape. Co-Authored-By: GPT-5.6 Sol <noreply@openai.com> Made-with: Cursor
|
Thanks for opening a PR! This project follows LLVM's incremental-development and AI-tool-use Before requesting review, please check that:
Reviewers are assigned through |
github-actions
Bot
added
the
large-pr
L2 Accuracy Results (EP vs CPU)
Threshold: 0.01 | Run: 3522 - Commit: |
MorphiZen EP Performance Results
EPContext Export Performance
EPContext Import Performance
OGA Benchmark Results
OGA Wheel Smoke (Python benchmark_e2e.py)
Run: 3522 - Commit: |
Remove Python numeric additions from the demonstration branch so the PR stays scoped to compiler changes and targeted IR coverage. Co-Authored-By: GPT-5.6 Sol <noreply@openai.com> Made-with: Cursor
Consolidate MatMul on one runtime ABI, align shape helper failure handling, and tighten compiler comments and LIT coverage. Co-Authored-By: GPT-5.6 Sol <noreply@openai.com> Made-with: Cursor
…apes Restore support for `[?, H, M, K] @ [?, H, K, N]`, which the strided-batch check rejected for any dynamic batch extent spanning more than one batch axis even though nothing broadcasts and one stride per operand is exact. Representability is now decided per axis, so only a provable partial broadcast is rejected. Extend the shared result-shape rule to reductions, which still computed their destination independently of `reifyResultShapes`: a positional dim copy is wrong under `keepdims = 0` once a reduced axis precedes a kept one, and gating the converter on the axes operand *count* while reification gated on it being *constant* made the two disagree for opset-13+ constant axes. One `computeReductionDimMap` now backs the static shape, the mixed shape, and result type inference, replacing ReduceProd's private copy of the rule. Split each shape category into a pure `infer*` function of static shapes and a `reify*` function that validates through it before touching the builder, so a failure cannot leave stray dim ops behind; `reifyGemmResultShape` previously emitted `tensor.dim` for A and B before validating the optional C. The split also gives `GemmOp` the static shape verification it lacked. Route the remaining `hip.max` / `hip.min` destinations through the broadcast helper, share the variadic pairwise chain between Max and Min, and derive each hipBLASLt batch stride by comparing the operand's matrix count against the output's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the sentinel error channel in `verifyHipOpShape`, where an empty outer vector meant "the callee already diagnosed", with a `FailureOr` returning helper. Both callers are single-destination ops, so the speculative one-shape-per-init generality goes away too and a verifier now pairs directly with its `infer*` rule instead of restating an adaptor lambda. Express "not known at compile time" as `std::optional` rather than a parallel `bool`: `resolveReductionAxes` returns an optional view over caller-owned storage, and `inferReduceResultType` / `createReductionEmptyTensor` take it directly, so no caller can pass axes that contradict a flag. Narrow the public surface to shape rules. The reduction dimension map and the static broadcast fold have no callers outside `HipShapeUtils.cpp` and become file-static; the map now uses `llvm::SmallBitVector` over the closed axis domain and `std::optional` in place of a `-1` sentinel, which was ambiguous next to ONNX negative-axis indices. Move the non-template destination helpers out of the header that all 87 conversion units include, and assert the right-alignment precondition whose unsigned subtraction would otherwise wrap silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
`reifyBroadcastShape` and `reifyReductionWithKeepdims` have no callers outside `HipShapeUtils.cpp`, so they join the dimension map and the static broadcast fold as file-static. Every remaining declaration in the header now has a real external consumer, and the header exposes only shape rules. Check every dimension in `createEmptyTensorFromReifiedShape` before materializing any index value: bailing out partway through left stray constants behind, which is what the surrounding no-stray-ops contract forbids. Guard `extractConstantIntVector` against the null Value that optional ONNX operands arrive as, since `getDefiningOp` would dereference it. Pass the operand's row and column extents to the batch-stride helper instead of a `function_ref` that materializes the product, which drops the lazily-invoked callback and the two lambdas at its call sites while keeping the matrix size off the paths that do not need it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
fhanuman
changed the title
`reduce_l2` landed on main after this branch diverged, carrying its own copy of the reduction dimension map — the same `outToIn` / `SmallSet` / `axesKnown` structure this branch removed from ReduceProd — and calling `inferReduceResultType` through the boolean-flag signature that no longer exists. The merge therefore did not compile, which is why CI failed on all three build jobs while each side built on its own. Route it through `resolveReductionAxes` and `createReductionEmptyTensor` like the other five reduction converters, so its destination and `reifyResultShapes` share one rule and `keepdims = 0` maps output dimensions back to the input dimensions they actually come from. No private copy of the dimension map remains in the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
Make rank-zero shape success unambiguous by returning FailureOr from the transpose, gather, GatherND, and shape-preserving reifiers, and validate all preconditions before emitting dimension IR. Route the corresponding converters through those shared rules, including MatMulNBits' N-derived output width. Close the dynamic partial-broadcast hole without aborting the host process: pass both operand batch counts to the MatMul runtime wrapper, reject counts that are neither one nor the output batch count, and report the failure through the existing device error flag so the generated interface returns a recoverable non-zero status to ORT. Also make verifier diagnostics reachable, normalize NoneType reduction axes, and align the documentation and LIT ABI coverage. Co-Authored-By: GPT-5.6 Sol <noreply@openai.com> Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
ONNX-to-HIP conversion and
reifyResultShapesindependently computed result extents. They could therefore disagree on the DPS destination shape for broadcast, Gemm, MatMul, reductions, Transpose, Gather/GatherND/GatherElements, and MatMulNBits. Reductions withkeepdims = 0could map dynamic output dimensions to the wrong input axes, while dynamic MatMul batch extents could conceal a partial per-axis broadcast that one constant stride cannot represent.Fix
Centralize each affected shape rule in
HipShapeUtilsand use it for both destination construction and reification; MatMul and Gemm also share the rule with their static verifiers. Reifiers now returnFailureOr, validate before emitting IR, and preserve rank-zero success. MatMul carries independent operand batch counts and strides to the runtime, which rejects dynamically concealed partial broadcasts through the existing recoverable error path instead of dispatching hipBLASLt with an invalid layout.