[PATCH] D25200: [InstCombine] New opportunities for FoldAndOfICmp and FoldXorOfICmp
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 18 16:27:39 PST 2016
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
More information about the llvm-commits
mailing list