Don't only do constant cases, also try cases were we can't constant
fold both sides. In the same we can't constant fold both sides, only
do transform if the select is single use.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This is not generally legal for div/rem, see https://alive2.llvm.org/ce/z/2iiR26 using one of your tests. This is only legal if the operation is speculatable.
Err, I probably misunderstood the question: There are no div/rem variants that are always speculatable, we can only speculate them if we know that the divisor is non-zero (or the more complex conditions for the signed case are met).
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
---|---|---|
191 | Comment that it returns MultiUse. | |
193 | Matching against all-wildcards is a bit silly :) | |
1011 | Please use isSafeToSpeculativelyExecute() here. This check is not sufficient for sdiv (due to INT_MIN / -1), and we don't want to duplicate logic. | |
1840 | Same here. | |
llvm/test/Transforms/InstCombine/binop-select.ll | ||
642–667 | Also test the straightforward case where the mul has a constant operand? Or is that already handled somewhere else (and if so, can we remove it)? |
llvm/test/Transforms/InstCombine/binop-select.ll | ||
---|---|---|
642–667 | hmm? Not sure what you mean by this comment. |
llvm/test/Transforms/InstCombine/binop-select.ll | ||
---|---|---|
642–667 | I mean something like (c ? x : C1) * C2, that isn't based on the select condition. |
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
---|---|---|
206 | Is this code in here so the case where the div is always folded away is always handled, as we're at worst going to relax to poison there and not introduce UB? If so, I think it might be more elegant to sink this check into FoldOpIntoSelect and use the isSafeToSpeculativelyExecute variant that accepts operands, so we can check the operation that we're actually going to introduce (and thus also skip the check if we're not going to introduce one). |
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
---|---|---|
206 |
Well, it looks like I imagined that variant... So ignore that bit (or leave a TODO for possible future improvement) and just take the bit about only checking it if we're going to introduce an op. |
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
---|---|---|
1834–1838 | Updatr comment? |
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp | ||
---|---|---|
206 | Without that variant we end up with regressions in: LLVM :: Transforms/InstCombine/div.ll LLVM :: Transforms/InstCombine/rem.ll I'm happy to just add the variant and use that, but I'm not sure what to do with load/call instructions. |
We generally fail to call 'foldBinOpIntoSelectOrPhi' in commonIDivTransforms and commonIRemTransforms
How hard would be to add support for PHIs in your shouldFoldOp helper? (Can be followup).
Well we match explicitly against m_Select(...) to check the all constant case so that wouldn't apply.
The speculative execution check can be reused, however.
Personally to add PHI support I'd create a new function "shouldFoldOpIntoX(...)" then have a variant
for select/phi and base the imm match on what X is.
I think this is basically okay if you drop the mul handling. As @xbolva00 pointed out (thanks, this is the bit I was missing here!) the mul case is already handled by foldBinOpIntoSelectOrPhi() and you're duplicating the transform for a special case now. If we want to extend handling to more cases, we should do so generically inside foldBinOpIntoSelectOrPhi(), not by repeating the transform with different arguments.
Ah, I had misunderstood his comment. I can move all the logics for div/mul/rem/etc.. into foldBinOpInstoSelectOrPhi. We actually call it for udiv as well (with really weak speculative checks).
(no rush) @nikic, I dropped the dependencies on the generic issafetospeculate... patch. Can take a look at this when you have the time.
Comment that it returns MultiUse.