W1.1 step 3a: the fragment materializer can mask a ragged edge - #510
Merged
Merged
Conversation
First concrete step of option (a) — restructuring producers onto the typed fragment contract. This is the prerequisite, not the producer change itself. §4.5 established why no producer could migrate: `fragment_pack` requires a `!tile.tile`, and `GenerateWMMAGemmKernel` supplies lane-level vectors it has already assembled element by element -- with `inb ? value : zero` masking for ragged edges. Handing that job to the compiler means the compiler has to be able to do it, and `materializeFragmentPack` could not: zero CmpIOp/SelectOp in its 126 lines, and an unguarded `vector.load` of `inputElementsPerLane` contiguous elements. A migrated producer would have read past the edge of every ragged matrix. `tile.view` is pointer-backed as (base, rowOrigin, colOrigin) and its ODS is already `Variadic<AnyType>`, so it now optionally carries (rowBound, colBound): the LOGICAL extents, runtime values on a ragged problem. No signature change. Out-of-bounds elements are always a contiguous TAIL, because the load walks the fast axis -- so `vector.create_mask` of the clamped remaining length is exact, and `maskedload` with a zero passthru reproduces the generator's `storeZero` element for element. That equivalence is what will let the migrated producer be bit-identical rather than merely close, which matters because bit-identity is the gate this work is verified against. The fast axis differs by ROLE and getting it backwards would silently zero live data: A is row-major so the load walks columns and the tail bound is the column extent; B is col-major (`col * leadingDim + row`) so it walks rows. Written down in the code and asserted in the fixture. One blocker found on the way: the source-layout guard required EXACTLY 3 view inputs, so the bounded form was unreachable -- and silently, since it fell into the generic "unsupported source layout" diagnostic rather than naming the arity. It now accepts 3 or 5 and treats anything else as malformed. Fixture drives the full typed chain (view -> fragment_pack a/b -> fragment_zero -> tile.mma -> fragment_unpack -> tile.store) through `lower-tile-to-rocm`, proving the bounded form masks and the 3-input form still emits a plain unmasked load. That second half matters most: every producer in the tree emits the 3-input form today, so a regression there is a silent codegen change on the working lane. Still to come for step 3: the producer restructure itself, and the accumulator threading in the typed branch (§4.4), which becomes testable once a producer exists. 302 lit, 14442 unit, mypy 0, ruff clean, docs in sync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f13d57ad0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const size_t viewInputs = view.getInputs().size(); | ||
| if ((role.getValue() != "a" && role.getValue() != "b") || !memory || | ||
| !layout || view.getInputs().size() != 3 || | ||
| !layout || (viewInputs != 3 && viewInputs != 5) || |
There was a problem hiding this comment.
When a shared producer emits this newly accepted five-operand form, the same valid Tile IR becomes backend-dependent: Tile_ViewOp still documents variadic, untyped inputs and its verifier accepts any three-or-more operands (TileOps.cpp:685-695), while the NVIDIA fragment materializer explicitly rejects every arity other than three (NVIDIALowering.cpp:3131-3137). Define and type-check (rowBound, colBound) in the shared tile.view contract, then either support them in affected fragment backends or record the required sibling outcomes; otherwise a portable producer migrated to this form will compile only through ROCm.
AGENTS.md reference: AGENTS.md:L81-L85
Useful? React with 👍 / 👎.
gstoner
pushed a commit
that referenced
this pull request
Aug 4, 2026
Correct, and it was a rule I have been applying elsewhere and missed here: I widened a SHARED IR op's accepted form inside one backend. Measured both halves of the report. `ViewOp::verify` accepted any count >= 3 for a pointer-backed view -- so a 4-operand view was legal and meaningless -- while `NVIDIALowering`'s materializer requires exactly 3. The same valid Tile IR therefore lowered on ROCm and was rejected on NVIDIA, with the shared verifier declining to say which form was real. The contract now lives in `ViewOp::verify`: exactly 3 `(base, rowOrigin, colOrigin)`, or 5 with `(rowBound, colBound)`, and anything else is TILE_VIEW_POINTER_ARITY. The ODS documents what the operands mean and that a backend which cannot mask must SAY so rather than ignore the bounds -- dropping them reads past the edge of the matrix. NVIDIA now emits NVFRAGMENT_BOUNDED_VIEW_UNSUPPORTED naming op and target (Decision #21) rather than folding the case into the generic arity message, which would have read as "malformed IR" for IR that is well-formed and merely unsupported there. ── One thing I could not verify, stated plainly ── The NVIDIA diagnostic is written and compiles; it has NOT been executed. The NVIDIA dialect is off by default in this build, and neither `--tessera-lower-to-gpu` nor `--tessera-nvidia-pipeline-sm120` reached the materializer with a bounded view on this host -- no diagnostic, no error. Verifying it needs `-DTESSERA_ENABLE_CUDA=ON`, and the numeric half needs an sm_120 host. Recorded as the NVIDIA sibling outcome rather than implied to work. The shared-verifier half IS verified: `tile_view_pointer_arity.mlir` rejects the 4- and 6-operand forms with the named code. 303 lit, 14442 unit, mypy 0, ruff clean, docs in sync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
First concrete step of option (a) — restructuring producers onto the typed fragment contract. This is the prerequisite, not the producer change itself.
Why this had to come first
§4.5 established why no producer could migrate:
fragment_packrequires a!tile.tile, andGenerateWMMAGemmKernelsupplies lane-level vectors it has already assembled element by element — withinb ? value : zeromasking for ragged edges.Handing that job to the compiler means the compiler has to be able to do it. It couldn't: zero
CmpIOp/SelectOpinmaterializeFragmentPack's 126 lines, and an unguardedvector.loadofinputElementsPerLanecontiguous elements. A migrated producer would have read past the edge of every ragged matrix.The change
tile.viewis pointer-backed as(base, rowOrigin, colOrigin)and its ODS is alreadyVariadic<AnyType>, so it now optionally carries(rowBound, colBound)— the logical extents, which are runtime values on a ragged problem. No signature change.Out-of-bounds elements are always a contiguous tail (the load walks the fast axis), so
vector.create_maskof the clamped remaining length is exact, andmaskedloadwith a zero passthru reproduces the generator'sstoreZeroelement for element. That equivalence is what will let the migrated producer be bit-identical rather than merely close — which matters, because bit-identity is the gate this work is verified against.The fast axis differs by role, and getting it backwards would silently zero live data: A is row-major so the load walks columns (tail bound = column extent); B is col-major (
col * leadingDim + row) so it walks rows. Written into the code and asserted in the fixture.A blocker found on the way
The source-layout guard required exactly 3 view inputs, so the bounded form was unreachable — and silently, since it fell into the generic
"unsupported source layout"diagnostic rather than naming the arity. It now accepts 3 or 5 and treats anything else as malformed.Fixture
Drives the full typed chain (
view → fragment_pack a/b → fragment_zero → tile.mma → fragment_unpack → tile.store) throughlower-tile-to-rocm, proving the bounded form masks and the 3-input form still emits a plain unmasked load.That second half matters most: every producer in the tree emits the 3-input form today, so a regression there is a silent codegen change on the working lane, not a new feature misbehaving.
Still to come for step 3
GenerateWMMAGemmKernel→ emit views + packs)302 lit, 14442 unit, mypy 0, ruff clean, docs in sync.
🤖 Generated with Claude Code