[PATCH] D44997: [InstCombine] Fold compare of int constant against a splatted vector of ints

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2470-2475
+  if (Bitcast->getType()->isIntegerTy()) {
+    Value *BCIOp = Bitcast->getOperand(0);
+    Value *Vec1 = nullptr;    // 1st vector arg of the shufflevector
+    Value *Vec2 = nullptr;    // 2nd vector arg of the shufflevector
+    Constant *Mask = nullptr; // Mask arg of the shufflevector
+    if (BCIOp->getType()->isIntOrIntVectorTy() &&
----------------
Early exit if the bitcast types aren't suitable:
  if (!Bitcast->getType()->isIntegerTy() || !Bitcast->getSrcTy()->isIntOrIntVectorTy())
    return nullptr;


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2478-2483
+      // Check whether every element of Mask is the same constant
+      ConstantInt *Elem = nullptr;
+      if (isa<ConstantAggregateZero>(Mask)) // zeroinitializer
+        Elem = Builder.getInt32(0);
+      else if (auto *CV = dyn_cast<ConstantDataVector>(Mask))
+        Elem = dyn_cast_or_null<ConstantInt>(CV->getSplatValue());
----------------
Constant::getSplatValue() ?


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2497-2501
+          // Which vector to extract from?
+          Value *Vec = Vec1;
+          const APInt &ElemVal = Elem->getValue();
+          if (ElemVal.uge(VecTy->getNumElements())) {
+            Vec = Vec2;
----------------
None of this is necessary AFAIK. A splat shuffle will be canonicalized so that the 2nd operand is undef and the mask constant is adjusted to choose from the 1st operand.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2509-2512
+          // If C isn't a pattern, and the pred is for (in)equality
+          // then we can fold the icmp into true/false.
+          Return = (Pred == ICmpInst::ICMP_NE) ? Builder.getTrue()
+                                               : Builder.getFalse();
----------------
Probably better to handle this part independently in InstSimplify (if it really matters?).


================
Comment at: test/Transforms/InstCombine/icmp-bc-vec.ll:193-201
+define i1 @test_i8_ne_pattern_5(<4 x i8> %invec) {
+; CHECK-LABEL: @test_i8_ne_pattern_5(
+; CHECK-NEXT:    ret i1 true
+;
+  %vec = shufflevector <4 x i8> %invec, <4 x i8> undef, <4 x i32> <i32 6, i32 6, i32 6, i32 6>
+  %cast = bitcast <4 x i8> %vec to i32
+  %cond = icmp ne i32 %cast, 1212696648
----------------
This test isn't changing with this patch. Move it to instsimplify if there's no existing coverage?
It would be easier to see the transforms if you check in all of the tests first based on trunk. Then, just run the update script again with this patch in place locally to rebase.


Repository:
  rL LLVM

https://reviews.llvm.org/D44997