[PATCH] D25200: [InstCombine] New opportunities for FoldAndOfICmp and FoldXorOfICmp

[Sanjay Patel via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:747-748
+struct BitGroupCheck {
+  // Whether this struct contains valid information or not.
+  bool Valid {false};
+  // If the Cmp, checks the bits in the group are nonzero?
----------------
Is the Valid field necessary? The callers can always just check if (!Mask) ?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:755-758
+// For an ICMP where RHS is zero, we want to check if the ICMP is equivalent to
+// comparing a group of bits in an integer value against zero.
+BitGroupCheck isAnyBitSet(Value *LHS, ICmpInst::Predicate CC) {
+
----------------
I've scanned over this a few times, and it just never reads clearly to me. Can you add comments and example patterns, so we'll know how this will be used?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:762-763
+
+  if (!Inst || (Inst && Inst->getOpcode() != Instruction::And))
+    return BGC;
+
----------------
This was not updated to address the earlier review comment.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:765
+
+  auto *Mask = dyn_cast<ConstantInt>(Inst->getOperand(1));
+  if (!Mask)
----------------
Please use m_APInt here or add a TODO comment because this won't work for vectors.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:776-778
+  case ICmpInst::ICMP_UGT:
+    BGC.CheckIfSet = true;
+    break;
----------------
The UGT case still has no test coverage?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:844-845
+  Value *Val = LHS->getOperand(0), *Val2 = RHS->getOperand(0);
+  auto *LHSCst = dyn_cast<ConstantInt>(LHS->getOperand(1));
+  auto *RHSCst = dyn_cast<ConstantInt>(RHS->getOperand(1));
+  if (!LHSCst || !RHSCst)
----------------
Same as above - use m_APInt or add a TODO about the lack of vector support.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:850
+
+  if (RHSCst->isZero() && LHSCst->isZero()) {
+
----------------
Can we check that RHSCst == LHSCst and remove the getBitWidth checks below?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:854-856
+    if (BGC1.Valid && BGC2.Valid && BGC1.CheckIfSet == BGC2.CheckIfSet &&
+        BGC1.Mask->getBitWidth() == BGC2.Mask->getBitWidth() &&
+        *BGC1.Mask == *BGC2.Mask && BGC1.Mask->isPowerOf2()) {
----------------
This needs a comment that shows the pattern we are matching and what it folds to.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:947
+  // and (y = and b, mask2) where both masks have one bit and mask1 >= mask2
+  if (RHSCst->isZero() && LHSCst->isZero()) {
+    BitGroupCheck BGC1 = isAnyBitSet(Val, LHSCC);
----------------
Can we check that RHSCst == LHSCst and remove the getBitWidth checks below?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:953-959
+      if (!BGC1.CheckIfSet &&
+          BGC1.Mask->countTrailingZeros() >=
+          BGC2.Mask->getBitWidth() - BGC2.Mask->countLeadingZeros() - 1)
+        return Builder->CreateICmp(ICmpInst::ICMP_ULT, Val, Val2);
+      else if (!BGC2.CheckIfSet &&
+          BGC2.Mask->countTrailingZeros() >=
+          BGC1.Mask->getBitWidth() - BGC1.Mask->countLeadingZeros() - 1)
----------------
This needs an explanatory comment with an example.


================
Comment at: test/Transforms/InstCombine/and-or-icmps.ll:54-56
+define i1 @test2(i32 %a, i32 %b) {
+; CHECK-LABEL: @test2(
+; CHECK-NEXT:    [[VAL1:%.*]] = and i32 %a, 8
----------------
Each test should have a comment to explain the motivation. Ie, it's good to have negative tests, but I can't tell from glancing at these why we would do the transform in some cases and skip it in the others.


https://reviews.llvm.org/D25200