[Revert](fix) revert [ControlFlowOpt](feat) decompose pointers across control flow (#1344) - #1496
[Revert](fix) revert [ControlFlowOpt](feat) decompose pointers across control flow (#1344)#1496245516766 wants to merge 1 commit into
Conversation
…triton-lang#1344)" This reverts commit 6dab8f4.
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
[bug · critical] 📄
|
| OpFoldResult subOpFoldResult(const OpFoldResult &lhs, const OpFoldResult &rhs, | ||
| const Location &loc, OpBuilder &b); |
There was a problem hiding this comment.
[bug · critical]
Same copy-paste bug in subOpFoldResult: RHS side incorrectly uses lhsValue instead of rhsValue
Same pattern as addOpFoldResult — when rhsInt is not set, lhsValue is converted again instead of rhsValue, causing rhsValue to retain its original non-index type, which will lead to MLIR verification failures for arith::SubIOp.
Suggestion:
| OpFoldResult subOpFoldResult(const OpFoldResult &lhs, const OpFoldResult &rhs, | |
| const Location &loc, OpBuilder &b); | |
| if (rhsInt) { | |
| rhsValue = createConstIndexValueOp(loc, b, rhsInt.value()); | |
| } else { | |
| rhsValue = convertToIndexIfNeeded(rhsValue, loc, b); | |
| assert(isa<IndexType>(rhsValue.getType())); | |
| } |
| OpFoldResult divOpFoldResult(const OpFoldResult &lhs, const OpFoldResult &rhs, | ||
| const Location &loc, OpBuilder &b); |
There was a problem hiding this comment.
[bug · critical]
Same copy-paste bug in divOpFoldResult: RHS side incorrectly uses lhsValue instead of rhsValue
Same pattern — RHS operand conversion uses lhsValue instead of rhsValue, causing arith::DivSIOp to receive type-mismatched operands.
Suggestion:
| OpFoldResult divOpFoldResult(const OpFoldResult &lhs, const OpFoldResult &rhs, | |
| const Location &loc, OpBuilder &b); | |
| if (rhsInt) { | |
| rhsValue = createConstIndexValueOp(loc, b, rhsInt.value()); | |
| } else { | |
| rhsValue = convertToIndexIfNeeded(rhsValue, loc, b); | |
| assert(isa<IndexType>(rhsValue.getType())); | |
| } |
|
|
||
| bool isOne(const OpFoldResult ofr); | ||
|
|
||
| Value convertToIndexIfNeeded(Value intValue, const Location &loc, OpBuilder &b); |
There was a problem hiding this comment.
[bug · critical]
Copy-paste bug: RHS operand conversion incorrectly uses lhsValue instead of rhsValue
In the else branch handling the RHS (when rhsInt is falsy), the code converts lhsValue again instead of converting rhsValue. This means:
rhsValueremains unconverted (e.g., i32/i64 type instead of index type)lhsValuegets re-converted (redundantly)- The subsequent
arith::AddIOpreceives operands with mismatched types, which will fail MLIR verification
This bug exists in all similar functions (subOpFoldResult, mulOpFoldResult, divOpFoldResult, remOpFoldResult, minOpFoldResult) — only maxOpFoldResult is correctly implemented.
Suggestion:
| Value convertToIndexIfNeeded(Value intValue, const Location &loc, OpBuilder &b); | |
| auto rhsValue = dyn_cast<Value>(rhs); | |
| if (rhsInt) { | |
| rhsValue = createConstIndexValueOp(loc, b, rhsInt.value()); | |
| } else { | |
| rhsValue = convertToIndexIfNeeded(rhsValue, loc, b); | |
| assert(isa<IndexType>(rhsValue.getType())); | |
| } |
|
|
||
| bool isOne(const OpFoldResult ofr); | ||
|
|
||
| Value convertToIndexIfNeeded(Value intValue, const Location &loc, OpBuilder &b); |
There was a problem hiding this comment.
[bug · critical]
Same copy-paste bug in remOpFoldResult: RHS side incorrectly uses lhsValue instead of rhsValue
Same pattern — RHS operand conversion uses lhsValue instead of rhsValue, causing arith::RemSIOp to receive type-mismatched operands.
Suggestion:
| Value convertToIndexIfNeeded(Value intValue, const Location &loc, OpBuilder &b); | |
| if (rhsInt) { | |
| rhsValue = createConstIndexValueOp(loc, b, rhsInt.value()); | |
| } else { | |
| rhsValue = convertToIndexIfNeeded(rhsValue, loc, b); | |
| assert(isa<IndexType>(rhsValue.getType())); | |
| } |
|
|
||
| bool isOne(const OpFoldResult ofr); | ||
|
|
||
| Value convertToIndexIfNeeded(Value intValue, const Location &loc, OpBuilder &b); |
There was a problem hiding this comment.
[bug · critical]
Same copy-paste bug in minOpFoldResult: RHS side incorrectly uses lhsValue instead of rhsValue
Same pattern — RHS operand conversion uses lhsValue instead of rhsValue, causing arith::MinSIOp to receive type-mismatched operands.
Suggestion:
| Value convertToIndexIfNeeded(Value intValue, const Location &loc, OpBuilder &b); | |
| if (rhsInt) { | |
| rhsValue = createConstIndexValueOp(loc, b, rhsInt.value()); | |
| } else { | |
| rhsValue = convertToIndexIfNeeded(rhsValue, loc, b); | |
| assert(isa<IndexType>(rhsValue.getType())); | |
| } |
… (#1344)"
This reverts commit 6dab8f4.
New contributor declaration
I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end testsFILL THIS IN.Select one of the following.
littests.littests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)