[PATCH] D153698: [InstCombine] canonicalize multi xor as cmp+select

[Allen zhong via Phabricator via llvm-commits]

Allen added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4279
+    // TODO: support more type operator
+    if (!isa<BinaryOperator>(V))
+      return V;
----------------
nikic wrote:
> These checks for BinaryOperator should not be necessary.
yes, but there is some regression. Does it make sense to extend this after we find some cases showed this is beneficial? 

a) the vector compare may has scalar operand, which will crash in above line 4268, such as func2 in file llvm/test/Transforms/LoopVectorize/same-base-access.ll.
```
  %17 = insertelement <4 x i32> %16, i32 %13, i64 3
  %18 = icmp slt <4 x i32> %17, <i32 4, i32 4, i32 4, i32 4>
```

b) there is many performance regression when enable **isa<SExtInst>(V)) and isa<ZExtInst>(V))** , such as case **lshr_out_of_range2** in file llvm/test/Transforms/InstCombine/shift.ll.

c) When I disable the **isa<SExtInst>(V)) and isa<ZExtInst>(V))**,  there are still some cases change because select instruction, where I'm also not sure if it's beneficial or not.
```
@@ -224,8 +224,8 @@ define i4 @PR45762(i3 %x4) {
 ; CHECK-NEXT:    [[T7:%.*]] = zext i3 [[T4]] to i4
 ; CHECK-NEXT:    [[ONE_HOT_16:%.*]] = shl nuw i4 1, [[T7]]
 ; CHECK-NEXT:    [[OR_69_NOT:%.*]] = icmp eq i3 [[X4]], 0
-; CHECK-NEXT:    [[UMUL_231:%.*]] = select i1 [[OR_69_NOT]], i4 0, i4 [[T7]]
-; CHECK-NEXT:    [[SEL_71:%.*]] = shl i4 [[ONE_HOT_16]], [[UMUL_231]]
+; CHECK-NEXT:    [[UMUL_231:%.*]] = shl i4 [[ONE_HOT_16]], [[T7]]
+; CHECK-NEXT:    [[SEL_71:%.*]] = select i1 [[OR_69_NOT]], i4 -8, i4 [[UMUL_231]]
 ; CHECK-NEXT:    ret i4 [[SEL_71]]
```



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

https://reviews.llvm.org/D153698