fix(ir): validate tile.load / tile.store shapes and valid_shape elements - #2570
Conversation
hw-native-sys#2565 gave the offsets tuple of tile.load / tile.store an element-wise integer-scalar check. The shapes and valid_shape tuples on the same two ops never got one, even though both op registrations declare them "TupleType of ScalarType" -- so the same silent miscompile stayed reachable through a different argument. A nested valid_shape makes InferWindowReadValidShape's obligations undecidable rather than false, so the "valid region fits the window" and "reads past the end" checks stop firing, and codegen lowers the tuple's last leaf: t = pl.load(a, [0, 0], [64, 64], [[200, 200], [64, 64]]) compiled clean on a [128, 128] source, emitting a tload of 200 rows into a 64-row tile: %t = pto.alloc_tile addr = %c0_i64 valid_row = %c200_index valid_col = %c64_index : !pto.tile_buf<..., rows=64, cols=64, ...> %a_pview = pto.partition_view %a_view, offsets = [%c0, %c0], sizes = [%c200_index, %c64_index] The plain-int spelling of the same request, valid_shape=[200, 64], is properly rejected. A nested shapes tuple instead lands in TileType.shape_ and dies 20 passes later in InitMemRef with an InternalError naming neither tile.load nor the nested tuple; tile.store's optional shapes tuple is accepted and silently ignored. Generalize the existing helper to ValidateIntScalarTupleElements(tuple, op_name, role) and apply it to all three remaining slots. The bar stays IsInt(), matching what tensor.slice enforces for its own shape tuple, so non-INDEX integer extents and symbolic dynamic extents remain legal. The keyword spelling (valid_shape=[[...]]) never becomes IR, so it never reached the deducer -- it failed in _normalize_expr with a bare "Cannot convert <class 'list'> to IR expression" that named neither the argument nor the mistake. Give that shared conversion point a nested-sequence branch, so both spellings now report something actionable.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change requires tile ChangesTile shape validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change adds early validation for malformed tile shape arguments and improves diagnostics without expanding runtime or security exposure; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Summary
tile.load'sshapes/valid_shapeandtile.store's optionalshapesis aScalarTypewith an integer dtype, closing the last three slots in the family fix(ir): validate that tile.load / tile.store offset elements are integer scalars #2565 startedCannot convert <class 'list'>Why
#2565 gave the
offsetstuple oftile.load/tile.storean element-wise check. Theshapesandvalid_shapetuples on the same two ops never got one, even though both op registrations declare them "TupleType of ScalarType" — so the same silent miscompile stayed reachable through a different argument.Nothing downstream compensates, by design.
InferWindowReadValidShape's obligations are defined only over integer scalars, so a non-scalar element makes them undecidable rather than false. The checks then pass, and codegen lowers whatever the element reduces to — its last leaf, for a nested tuple.A realistic way to write it: reading
valid_shapeas per-dim[start, extent]pairs, mirroring theoffsets/shapespair right before it.With plain ints,
valid_shape=[200, 64]is rejected bytile.loaditself:With the extra bracket level it compiled clean, all the way to valid PTO:
— an out-of-bounds GM read and an out-of-bounds store, no diagnostic at any layer.
The benign case is arguably worse:
valid_shape=[[0, 40], [0, 56]]emitsvalid_row = 40, valid_col = 56, which looks correct and quietly confirms the wrong mental model.The other two slots fail less catastrophically but still badly. A nested
shapeslands inTileType.shape_and dies 20 passes later inInitMemRefwithInternalError: requires static shape for variable 't__ssa_v0' ... Fix the upstream op— naming neithertile.loadnor the nested tuple.tile.store's optionalshapesis accepted and silently ignored.Notes for reviewers
The bar stays
IsInt(), matching whattensor.sliceenforces for its ownshapetuple. Non-INDEX integer extents (INT32) and symbolic dynamic extents remain legal — both are covered by a test, since a stricter bar would break ordinary dynamic-shape kernels.Only the positional spelling reached the deducer. The DSL parser turns a list literal into a
MakeTuplerecursively;_wrap_argunwraps exactly one level, so innerMakeTuples arrive asir.Exprand sail through_to_make_tuple. Spelled as a kwarg (valid_shape=[[...]]) the list never becomes IR at all — it died in_normalize_exprwithCannot convert <class 'list'> to IR expression, an op-agnosticTypeErrornaming neither the argument nor the mistake. Two spellings of one mistake, two unrelated behaviours. This adds a nested-sequence branch at that shared conversion point, so both now report something actionable. Nothing in-tree matched on the old message.Helper rename.
ValidateOffsetTupleElementsbecomesValidateIntScalarTupleElements(tuple, op_name, role);rolenames the operand with the spelling the DSL exposes (offset/shapes/valid_shape) so the message points at the argument the author wrote. Existingoffsetmessages are unchanged.DSL tests use an
Any-annotated closure var, as the offsets tests do — pyright rejects a nested list literal statically, so the value's shape has to be knowable only at parse time for the test to exercise what a checker cannot catch.Testing
python -m pytest tests/ut/ir/operators/test_tile_ops.py -k "TileLoadStoreShapeElements or TileLoadStoreOffsetElements" -n "$PYPTO_TEST_JOBS" -v— 17 passed (10 new)python -m pytest tests/ut/ tests/lint/ -n "$PYPTO_TEST_JOBS"— 10787 passed, 3 skipped, 1 xfailedpython -m pytest tests/st/codegen -n "$PYPTO_TEST_JOBS"— 60 passedctest --parallel "$PYPTO_TEST_JOBS"— 1/1 passedpre-commit run --files src/ir/op/tile_ops/memory.cpp python/pypto/ir/utils.py python/pypto/ir/op/tile_ops.py python/pypto/language/op/tile_ops.py tests/ut/ir/operators/test_tile_ops.py