[PATCH] D71568: [InstCombine] `select + mul` -> `select + shl` with power of twos.

[Danila Kutenin via Phabricator via llvm-commits]

danlark added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:265
+// X mul 2^C -> X << C
+static Instruction *foldMulPow2Cst(Value *Op0, Value *Op1,
+                                   const BinaryOperator &I, InstCombiner &IC) {
----------------
lebedev.ri wrote:
> I think it would be better to change Op1 type to `Constant`, and make the caller do the cast.
It would be harder to combine OperandFoldActions then in udiv and mul at the same time, though I changed to only one call


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:320-325
+  // X mul (C1 << N), where C1 is "1<<C2"  -->  X << (N+C2)
+  if (match(Op1, m_Shl(m_Power2(), m_Value())) ||
+      match(Op1, m_ZExt(m_Shl(m_Power2(), m_Value())))) {
+    Actions.push_back(OperandFoldAction(foldMulShl, Op1));
+    return Actions.size();
+  }
----------------
lebedev.ri wrote:
> If i'm reading this correctly, these are miscompilations:
> https://rise4fun.com/Alive/IKH
Yeah, thank you for the tool. if Shl NSW, this should not happen and this is correct I believe https://rise4fun.com/Alive/sog


================
Comment at: llvm/test/Transforms/InstCombine/mul.ll:669-682
 define i32 @shift_if_power2_double_select(i32 %x, i32 %y, i1 %cond1, i1 %cond2) {
 ; CHECK-LABEL: @shift_if_power2_double_select(
-; CHECK-NEXT:    [[SHL_RES:%.*]] = shl i32 8, [[Y:%.*]]
-; CHECK-NEXT:    [[SEL1:%.*]] = select i1 [[COND1:%.*]], i32 [[SHL_RES]], i32 1024
-; CHECK-NEXT:    [[SEL2:%.*]] = select i1 [[COND2:%.*]], i32 16, i32 [[SEL1]]
-; CHECK-NEXT:    [[R:%.*]] = mul nuw i32 [[SEL2]], [[X:%.*]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[Y:%.*]], 3
+; CHECK-NEXT:    [[DOTV:%.*]] = select i1 [[COND1:%.*]], i32 [[TMP1]], i32 10
+; CHECK-NEXT:    [[R_V:%.*]] = select i1 [[COND2:%.*]], i32 4, i32 [[DOTV]]
+; CHECK-NEXT:    [[R:%.*]] = shl nuw i32 [[X:%.*]], [[R_V]]
 ; CHECK-NEXT:    ret i32 [[R]]
----------------
lebedev.ri wrote:
> This is a miscompile:
> ```
> ----------------------------------------
> define i32 @shift_if_power2_double_select(i32 %x, i32 %y, i1 %cond1, i1 %cond2) {
> %0:
>   %shl.res = shl i32 8, %y
>   %sel1 = select i1 %cond1, i32 %shl.res, i32 1024
>   %sel2 = select i1 %cond2, i32 16, i32 %sel1
>   %r = mul nuw i32 %x, %sel2
>   ret i32 %r
> }
> =>
> define i32 @shift_if_power2_double_select(i32 %X, i32 %Y, i1 %COND1, i1 %COND2) {
> %0:
>   %TMP1 = add i32 %Y, 3
>   %DOTV = select i1 %COND1, i32 %TMP1, i32 10
>   %R_V = select i1 %COND2, i32 4, i32 %DOTV
>   %R = shl nuw i32 %X, %R_V
>   ret i32 %R
> }
> Transformation doesn't verify!
> ERROR: Target is more poisonous than source
> 
> Example:
> i32 %x = #x0f360904 (255199492)
> i32 %y = #x0000001d (29)
> i1 %cond1 = #x1 (1)
> i1 %cond2 = undef
> 
> Source:
> i32 %shl.res = #x00000000 (0)
> i32 %sel1 = #x00000000 (0)
> i32 %sel2 = #x00000000 (0)      [based on undef value]
> i32 %r = #x00000000 (0)
> 
> Target:
> i32 %TMP1 = #x00000020 (32)
> i32 %DOTV = #x00000020 (32)
> i32 %R_V = #x00000020 (32)
> i32 %R = poison
> Source value: #x00000000 (0)
> Target value: poison
> 
> Summary:
>   0 correct transformations
>   1 incorrect transformations
>   0 errors
> 
> ```
Same issue with nsw for shift, fixed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71568/new/

https://reviews.llvm.org/D71568