fix(codegen): Infer static v_row/v_col when source valid >= slice size#1303
fix(codegen): Infer static v_row/v_col when source valid >= slice size#1303Hzfengsy merged 1 commit intohw-native-sys:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refines static-dimension inference in InferSubviewTileTypeComponents by adding two early-return branches that mark subview dimensions statically known when (1) offset==0 and requested size equals source valid extent, or (2) source valid constant ≥ requested size. ChangesSubview Dimension Static Inference Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the InferSubviewTileTypeComponents function in src/backend/common/pto_ops_common.cpp to improve the static inference of tile type components. Specifically, it adds logic to identify cases where the number of valid elements is statically known because the source's valid extent is greater than or equal to the requested size, independent of the offset. It also adds a missing return statement and clarifying comments to the existing logic. There were no review comments provided, so I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/common/pto_ops_common.cpp (1)
249-255: ⚡ Quick winDocument (or assert) the bounds invariant that makes Case 3 sound.
Case 3 concludes
v_row/v_col = size(static) whenevervalid_const->value_ >= size, even though the offset may be a dynamic runtime value. This inference is only correct under the implicit invariant:offset + size <= valid_const->value_If that invariant does not hold (e.g.,
offset = valid - size + 1), then the true remaining valid extent ismin(size, valid - offset) < size, but the emitted tile-buf type would claimsize. PTOAS would then accept the program with an incorrect type annotation rather than flagging the out-of-bounds access.The comment currently says "Any valid offset leaves exactly
sizevalid elements" — but "valid offset" is doing all the work; the word "valid" should be unpacked explicitly.💡 Suggested improvement: make the invariant explicit
- // Any valid offset leaves exactly `size` valid elements in this dimension, - // so v_row/v_col is statically known regardless of the offset value. - if (valid_const && valid_const->value_ >= size) { + // When the source valid extent is statically >= size, any in-bounds offset + // (i.e., offset + size <= valid_const->value_, guaranteed by IR well-formedness) + // leaves exactly `size` valid elements. v_row/v_col is therefore statically known. + DCHECK(!offset_const) // dynamic-offset path; static-offset already handled by Case 1 + << "InferSubviewTileTypeComponents: Case 3 should not be reached with a constant offset"; + if (valid_const && valid_const->value_ >= size) {Alternatively, add a
DCHECKorINTERNAL_CHECKthatoffset_const == nullptrhere (since a constant offset with a constant valid would have been caught by Case 1), and add a brief note that the caller is responsible for ensuring the subview is in-bounds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/common/pto_ops_common.cpp` around lines 249 - 255, The code assumes that when valid_const->value_ >= size it is safe to mark *out_value = size and *out_dynamic = false; make this invariant explicit or assert it: add a comment stating the required precondition "offset + size <= valid_const->value_" (or that the caller guarantees a valid subview) and/or add a runtime DCHECK/INTERNAL_CHECK on offset_const (e.g., ensure offset_const == nullptr or verify the numeric invariant when offset is available) so Case 3 in the logic around valid_const, offset_const, size, out_value, out_dynamic cannot emit an over-sized tile-buf type accepted by PTOAS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/backend/common/pto_ops_common.cpp`:
- Around line 249-255: The code assumes that when valid_const->value_ >= size it
is safe to mark *out_value = size and *out_dynamic = false; make this invariant
explicit or assert it: add a comment stating the required precondition "offset +
size <= valid_const->value_" (or that the caller guarantees a valid subview)
and/or add a runtime DCHECK/INTERNAL_CHECK on offset_const (e.g., ensure
offset_const == nullptr or verify the numeric invariant when offset is
available) so Case 3 in the logic around valid_const, offset_const, size,
out_value, out_dynamic cannot emit an over-sized tile-buf type accepted by
PTOAS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a149d8a4-39c5-4d57-9035-a59f1ff23855
📒 Files selected for processing (1)
src/backend/common/pto_ops_common.cpp
When the source tile's valid extent in a dimension is a static constant that is >= the requested slice size, the sliced valid extent is always exactly `size` regardless of the (possibly dynamic) offset. Add this as a third inference case in InferSubviewTileTypeComponents. Also fixes a missing return after the zero-offset equivalence case, which would fall through into the new case without it.
2 participants
Summary
InferSubviewTileTypeComponents(src/backend/common/pto_ops_common.cpp): when the source tile's valid extent in a dimension is a static constant>= slice size, the sliced valid extent is statically known assizeregardless of the (possibly dynamic) offset.return;after the zero-offset equivalence case (Case 2), which would have fallen through into the new case.Background
pto.subviewwith a dynamic loop offset (e.g.k0 = i * K) was emitting dynamicv_row/v_coleven when the source tile is large enough that any valid offset leaves exactlysizevalid elements. This caused PTOAS to reject otherwise-valid programs. The new inference case handles this: ifsource_valid >= slice_size(both static constants), the subview valid extent is triviallysize.Testing