Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Needs more tests for preconditions, e.g. what happens if the constant is not 1?
Note that there is also a more general form of this fold, see https://github.com/llvm/llvm-project/blob/697a162fa63df328ec9ca334636c5e85390b2bf0/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp#L806. The tricky bit here is that we would nee X < 2*Y, which is hard to establish as a precondition in this context.
| llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
|---|---|---|
| 1791 | Why the nuw requirement? Seems to be fine without it? https://alive2.llvm.org/ce/z/uo7HMz | |
| 1792 | Why is this specific to assumes? Can we do a generic simplifyICmp? (If we do so, Op0 needs to be frozen.) | |
Address comments:
a) check the assume with more generate API simplifyICmpInst
b) Add 3 test cases, including some Negative tests
c) Fix the checking and only do this when the return value is isOneValue()
d) Add the logic of frozen
| llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
|---|---|---|
| 1792 | Would this be better? I always hate to see isa<> + cast<> pairs - it feels like we're duplicating work. if (auto *CVal = dyn_cast_or_null<Constant>(Val)) {
if (CVal->isOneValue()) {
}
} | |
| llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
|---|---|---|
| 1792 | Apply your review, thanks for your detailed example. | |
| llvm/include/llvm/Analysis/InstructionSimplify.h | ||
|---|---|---|
| 244 ↗ | (On Diff #497851) | Why the new parameter? You can use SQ.withInstruction(I) to change the context instruction. |
| llvm/include/llvm/Analysis/InstructionSimplify.h | ||
|---|---|---|
| 244 ↗ | (On Diff #497851) | Thanks your review, I update with SQ.getWithInstruction(&I). |
| llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
|---|---|---|
| 1793–1794 | Use match(Val, m_One()) instead? | |
| llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll | ||
| 24 | Drop noundef attributes | |
| 77 | Missing tests where add constant is not 1. It would be good to also have a test that is not based on assume, where %x < %n is known through some other way instead. | |
Address comments
- use match(Val, m_One()) to replace CVal->isOneValue()
- Drop the attribute noundef of function urem_assume_without_nuw, then it will see freeze
- Add a new negative case urem_assume_with_unexpected_const
| llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
|---|---|---|
| 1789 | This comment doesn't match the code. // For "(X + 1) % Op1" and if (X u< Op1) => (X + 1) == Op1 ? 0 : X + 1 | |
| 1794 | This is called FrozenX, but it is freezing Op0. Should it freeze X directly as the name suggests? | |
| llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll | ||
| 77 | Still no tests with no "assume" call. There should be a way to show the transform using math or bitwise logic ops and/or using a dominating condition? | |
Address comments:
a) Adding a new case urem_without_assume
b) Update the code comment to match the code
c) Rename the FreezeX to FreezeOp0 as the name suggests
| llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll | ||
|---|---|---|
| 5 | Should also drop noundef here and on the return values. | |
| 105 | %cmp is a dead instruction here. The request wasn't to just drop the assume, but to establish the %x < %n precondition without using one. It's a bit tricky to do this without also triggering other folds, but maybe this would work? define i64 @test(i64 %arg) {
%x = shl nuw i64 1, %arg
%n = shl nuw i64 3, %arg
%add = add i64 %x, 1
%out = urem i64 %add, %n
ret i64 %out
}Unfortunately this example still doesn't show the need for freeze (https://alive2.llvm.org/ce/z/FJCdt4) ... I'm pretty sure it's needed in the general case, but I'm not sure how to come up with an example. | |
| llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll | ||
|---|---|---|
| 43 | If it's not changing behavior of the optimization (and it is not in any cases here from what I see), use i8 or possibly even narrower type widths. This makes it more likely that Alive2 can run quickly on all tests to verify correctness. | |
| llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll | ||
|---|---|---|
| 105 | If I'm understanding your question correctly, the following demonstrates the need for freeze. define i8 @src(i8 %arg, i8 %arg2) {
%x = urem i8 %arg, %arg2
%add = add i8 %x, 1
%out = urem i8 %add, %arg2
ret i8 %out
}
define i8 @tgt(i8 %arg, i8 %arg2) {
%xx = urem i8 %arg, %arg2
%x = freeze i8 %xx
%add = add i8 %x, 1
%cmp = icmp eq i8 %add, %arg2
%out = select i1 %cmp, i8 0, i8 %add
ret i8 %out
} | |
| llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll | ||
|---|---|---|
| 105 | Thanks, that's exactly what I was looking for! | |
Address comments:
a) Drop all the attribute noundef on the argument and return value
b) Change all the value type to i8 as Alive2 can run quickly on all tests to verify correctness
c) Update case urem_without_assume
This comment doesn't match the code.
Should it be: