Now select + mul transforms to select + shl with power of twos known in select.
Patch by Danila Kutenin. email:danilak at google.com, github:danlark1@ . I don't have commit rights.
Differential D71568
[InstCombine] `select + mul` -> `select + shl` with power of twos. danlark on Dec 16 2019, 1:50 PM. Authored by
Details
Now select + mul transforms to select + shl with power of twos known in select. Patch by Danila Kutenin. email:danilak at google.com, github:danlark1@ . I don't have commit rights.
Diff Detail
Event Timeline
Comment Actions undef should return zero in select+shl
Comment Actions Thank you for working on this.
Comment Actions Fix overflow in shifting and further propagation
Comment Actions I want to say sorry if I am asking/doing stupid things, I am only getting used to things Comment Actions Sorry, i lost track of this patch.
Comment Actions There's a lot going on here, and the patch doesn't apply cleanly to current source. Can you pre-commit any NFC changes/tests to make this patch smaller? For example, the udiv change/tests are not affected by the mul code? Comment Actions Sorry - I didn't see earlier that this patch is part of a sequence. Comment Actions I am not sure but I just reused the code for both division and multiplication -- from my perspective the idea is the same. Also, I rebased a bit and made the patch smaller as you asked. Comment Actions So, I basically don't see any reason division machine is doing a lot of things but as abstractions and semantics are mostly the same, I decided to reuse it. And to generalize power 2 multiplication then. That's true that most of patterns are just one mul(select) but I saw at least one where the depth level was two. Comment Actions Yes, I understand the similarity, I'm just not sold on the idea that we ever needed the div complexity in the first place. If @lebedev.ri is comfortable with all of the poison-related corner cases, then we can proceed. AFAIK, there are no active contributers on this code, so if you've looked it over - then you're the expert. :) Comment Actions I'm struggling to keep all of the poison potential straight. Can we make this patch only include foldMulPow2Cst() (remove foldMulShl())? Double-check to make sure I've translated this correctly: define i32 @shift_if_power2_double_select_zext(i32 %x, i32 %y, i1 %cond1, i1 %cond2) { %shl = shl nsw i32 2147483648, %y %sel1 = select i1 %cond1, i32 %shl, i32 1024 %sel2 = select i1 %cond2, i32 16, i32 %sel1 %r = mul nsw i32 %x, %sel2 ret i32 %r } $ opt -instcombine mul.ll -S define i32 @shift_if_power2_double_select_zext(i32 %x, i32 %y, i1 %cond1, i1 %cond2) { %1 = add i32 %y, 31 %.v = select i1 %cond1, i32 %1, i32 10 %r.v = select i1 %cond2, i32 4, i32 %.v %r = shl nsw i32 %x, %r.v ret i32 %r } Miscompile? |
You really want to only use Constant::replaceUndefsWith() to fixup that single case,
not pessimize everything that uses this generic utility..