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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Thank you for working on this.



================
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) {
----------------
I think it would be better to change Op1 type to `Constant`, and make the caller do the cast.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:268-269
+  Constant *C1 = getLogBase2(Op0->getType(), cast<Constant>(Op1));
+  C1 = Constant::replaceUndefsWith(
+      C1, ConstantInt::get(C1->getType()->getScalarType(), 0));
+  if (!C1)
----------------
Add a comment explaining that we really do need to sanitize `undef` to `0` here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:281
+// X mul (C1 << N), where C1 is "1<<C2"  -->  X << (N+C2)
+// X mul (zext (C1 << N)), where C1 is "1<<C2"  -->  X << (N+C2)
+static Instruction *foldMulShl(Value *Op0, Value *Op1, const BinaryOperator &I,
----------------
// X mul (zext (C1 << N)), where C1 is "1<<C2"  -->  X << (zext (add N, C2))



================
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();
+  }
----------------
If i'm reading this correctly, these are miscompilations:
https://rise4fun.com/Alive/IKH


================
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]]
----------------
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

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71568