feat(ops): add B02 TSELS and TPRELU support - #2182
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 PR replaces ChangesSelection and PReLU operation updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TileSelsWrapper
participant IR
participant PTOCodegen
Caller->>TileSelsWrapper: call sels(mask, src, tmp, scalar)
TileSelsWrapper->>IR: create tile.sels operation
IR->>PTOCodegen: emit four-operand tsels
PTOCodegen-->>Caller: generated operation
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: a4d7e26ec8
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/pypto/ir/utils.py (1)
331-345: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSilent float→int truncation when
retype_constants=Trueon an integer operand.With retyping enabled, a float literal paired with an integer tile no longer takes the float fallback, so
_const_at_dtyperunsint(value):sels(mask, int32_tile, tmp, -1.5)silently becomes-1. Previously this combination surfaced as a dtype mismatch. Consider rejecting non-integral floats on integer targets instead of truncating.🛡️ Proposed guard
if target.is_float() or target.is_int(): + if target.is_int() and isinstance(value, float) and not float(value).is_integer(): + raise ValueError( + f"Scalar {value} cannot be represented exactly in {target}; " + "use an integer literal or cast the operand." + ) return _const_at_dtype(value, target, span)🤖 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/utils.py` around lines 331 - 345, Update the constant-typing logic before _const_at_dtype so retype_constants=True does not convert a non-integral float to an integer target. Reject or preserve the existing dtype-mismatch behavior for float values with fractional parts when target.is_int(), while allowing integral floats and leaving other promotion paths unchanged.
🧹 Nitpick comments (1)
src/ir/op/tile_ops/elementwise.cpp (1)
918-932: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRows check uses physical extent while cols check uses valid extent.
required_rowsis compared againsttmp_type->shape_[0]butrequired_colsagainsttmp_valid_shape[1]. If that mixed contract is deliberate, a one-line comment would help; otherwise both should read from the same extent source. The literal8also duplicates the packed-predicate bits-per-byte constant used inMakePackedPredicateTileType; hoisting it to a file-scopeconstexprwould keep the two paths in sync.🤖 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 918 - 932, The UINT8 validation in the surrounding elementwise operator uses inconsistent physical versus valid extents and duplicates the packed-predicate bit-width literal. Align the row and column checks to the intended extent contract, document the choice if the difference is deliberate, and replace the literal 8 in required_cols and MakePackedPredicateTileType with one shared file-scope constexpr.
🤖 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.
Outside diff comments:
In `@python/pypto/ir/utils.py`:
- Around line 331-345: Update the constant-typing logic before _const_at_dtype
so retype_constants=True does not convert a non-integral float to an integer
target. Reject or preserve the existing dtype-mismatch behavior for float values
with fractional parts when target.is_int(), while allowing integral floats and
leaving other promotion paths unchanged.
---
Nitpick comments:
In `@src/ir/op/tile_ops/elementwise.cpp`:
- Around line 918-932: The UINT8 validation in the surrounding elementwise
operator uses inconsistent physical versus valid extents and duplicates the
packed-predicate bit-width literal. Align the row and column checks to the
intended extent contract, document the choice if the difference is deliberate,
and replace the literal 8 in required_cols and MakePackedPredicateTileType with
one shared file-scope constexpr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 303255eb-bb59-47b7-830d-e72a1c4180c9
📒 Files selected for processing (19)
docs/en/dev/passes/31-memory_reuse.mddocs/en/dev/ptoas-op-status.mddocs/en/user/02-operation_reference.mddocs/zh-cn/dev/passes/31-memory_reuse.mddocs/zh-cn/dev/ptoas-op-status.mddocs/zh-cn/user/02-operation_reference.mdpython/pypto/debug/torch_codegen.pypython/pypto/ir/op/tile_ops.pypython/pypto/ir/utils.pypython/pypto/language/op/tile_ops.pysrc/backend/common/pto_ops_elementwise.cppsrc/ir/op/tile_ops/elementwise.cpptests/st/runtime/ops/test_activation_ops.pytests/st/runtime/ops/test_prelu.pytests/st/runtime/ops/test_sels.pytests/ut/codegen/test_pto_codegen_ops.pytests/ut/debug/test_torch_codegen.pytests/ut/ir/operators/test_tile_ops.pytests/ut/ir/transforms/test_memory_reuse.py
|
Review follow-up: 9dceb8d also rejects non-integral floating constants before integer retyping. The PReLU scratch check intentionally uses physical rows (the extra hardware scratch row) and valid columns (usable packed-predicate bytes); the shared 8-bit packing constant and an explanatory comment now make that distinction explicit. |
1 participant
Summary
tile.selspath with the canonicalmask, src, tmp, scalarTSELS contracttile.prelu(src, slope, tmp)inference, backend emission, alias rules, debug codegen, and bilingual documentationValidation
cmake --build build --parallel 1--jobs=1: PASStask_20260728_033948_204201930367Platform status