feat(codegen): validate remainder op family - #2186
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe remainder family now uses stricter dtype, shape, valid-region, scalar, and scratch-tile contracts. Backend layout and memory-reuse handling are updated, with added unit, codegen, runtime, and hardware-status coverage. ChangesRemainder family
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant RemainderCase
participant TileOps
participant PTOCodegen
participant A2A3Hardware
TestRunner->>RemainderCase: create operation-specific case
RemainderCase->>TileOps: build rem, rems, fmod, or fmods
TileOps->>PTOCodegen: validate and lower remainder operation
PTOCodegen->>A2A3Hardware: execute exact PTO instruction
A2A3Hardware-->>TestRunner: return valid-region results
Possibly related PRs
Suggested labels: 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 |
![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: cb73445fff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ir/op/tensor_ops/elementwise.cpp (1)
296-314: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "floating-point remainder" description now that integer dtypes are supported.
DeduceTensorRemainderType/DeduceTensorScalarRemainderTyperoute throughIsRemainderDataType, which now acceptsINT16,UINT16,INT32,UINT32in addition toFP16/FP32. Thetensor.fmod/tensor.fmodsset_description(...)text still reads "floating-point remainder", which is misleading for the newly-supported integer paths. This mirrors the same drift intile.fmods(src/ir/op/tile_ops/elementwise.cpp);tile.fmod's description was correctly updated to "truncating remainder" but the scalar variants and tensor level were missed.🤖 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/ir/op/tensor_ops/elementwise.cpp` around lines 296 - 314, Update the set_description text for tensor.fmod and tensor.fmods to describe a truncating remainder rather than a floating-point remainder, matching the integer-capable behavior and the existing tile.fmod terminology.src/ir/op/tile_ops/elementwise.cpp (1)
647-660: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "floating-point remainder" description; see companion comment in
src/ir/op/tensor_ops/elementwise.cpp.
tile.fmodsnow routes throughDeduceTileScalarRemainderType, which accepts integer dtypes viaIsRemainderDataType, but itsset_description(line 649) is unchanged and still says "floating-point remainder" — inconsistent with the correctly-updatedtile.fmoddescription right above it ("truncating remainder ... with matching dtype and shape").🤖 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/ir/op/tile_ops/elementwise.cpp` around lines 647 - 660, Update the set_description for the tile.fmods registration to describe a truncating remainder with matching dtype and shape, consistent with tile.fmod and the integer-capable DeduceTileScalarRemainderType behavior; leave the registration and type-deduction logic unchanged.
🧹 Nitpick comments (1)
python/pypto/ir/op/tile_ops.py (1)
837-838: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSync the
pl.tile.*wrapper docstrings with the corrected IR contracts.
python/pypto/language/op/tile_ops.pystill documentsrem/remstmpas a same-shape/temporary hardware tile, andfmod/fmodsas “IEEE-style ... matchingtorch.fmod”. Update these wrappers to match the corrected docs/tests: describe the actual valid-row/valid-columntmprequirements forrem/rems, and describefmod/fmodsas truncating remainders matchingtorch.fmodwith the dividend signing.🤖 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 `@python/pypto/ir/op/tile_ops.py` around lines 837 - 838, Update the pl.tile.* wrapper docstrings in the rem/rems and fmod/fmods definitions: document tmp using the actual valid-row and valid-column requirements from the corrected IR contract, rather than same-shape or temporary-hardware wording, and describe fmod/fmods as truncating remainders matching torch.fmod with dividend signing instead of “IEEE-style.”
🤖 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.
Inline comments:
In `@src/ir/op/tile_ops/elementwise.cpp`:
- Around line 301-332: Update CheckRemainderTileContract’s physical-shape-rank
and valid-shape-rank CHECK messages to include the actual lhs and rhs rank
values, using the existing “but got X and Y” diagnostic pattern from sibling
contract checks. Leave the dimension-level validation and other checks
unchanged.
---
Outside diff comments:
In `@src/ir/op/tensor_ops/elementwise.cpp`:
- Around line 296-314: Update the set_description text for tensor.fmod and
tensor.fmods to describe a truncating remainder rather than a floating-point
remainder, matching the integer-capable behavior and the existing tile.fmod
terminology.
In `@src/ir/op/tile_ops/elementwise.cpp`:
- Around line 647-660: Update the set_description for the tile.fmods
registration to describe a truncating remainder with matching dtype and shape,
consistent with tile.fmod and the integer-capable DeduceTileScalarRemainderType
behavior; leave the registration and type-deduction logic unchanged.
---
Nitpick comments:
In `@python/pypto/ir/op/tile_ops.py`:
- Around line 837-838: Update the pl.tile.* wrapper docstrings in the rem/rems
and fmod/fmods definitions: document tmp using the actual valid-row and
valid-column requirements from the corrected IR contract, rather than same-shape
or temporary-hardware wording, and describe fmod/fmods as truncating remainders
matching torch.fmod with dividend signing instead of “IEEE-style.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50b28204-992c-4a29-8780-89970407f51c
📒 Files selected for processing (14)
docs/en/dev/passes/31-memory_reuse.mddocs/en/dev/ptoas-op-status.mddocs/zh-cn/dev/passes/31-memory_reuse.mddocs/zh-cn/dev/ptoas-op-status.mdpython/pypto/ir/op/tile_ops.pysrc/backend/common/pto_ops_elementwise.cppsrc/ir/op/tensor_ops/elementwise.cppsrc/ir/op/tile_ops/elementwise.cpptests/st/runtime/ops/test_bitwise.pytests/st/runtime/ops/test_remainder.pytests/ut/codegen/test_pto_codegen_ops.pytests/ut/ir/operators/test_tensor_ops.pytests/ut/ir/operators/test_tile_ops.pytests/ut/ir/transforms/test_memory_reuse.py
7b1300d to
ff882cf
Compare
1 participant
Summary
tile.rem,tile.rems,tile.fmod, andtile.fmodswith the current PTO-ISA dtype, shape, valid-region, scratch, layout, and aliasing contractsValidation
git diff --check: passed/data/chenshenai/test2: passed.ptofiles contain exactpto.trem,pto.trems,pto.tfmod, andpto.tfmodsopsA5 hardware validation remains pending and is documented in the status matrix.