[PATCH] D52596: [InstCombine] Add combines of icmp (mul nsw/nuw X, C2), C when C % C2 == 0

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

There's enough going on here that I'd prefer to split this patch up further. Can you start by handling just the eq/ne predicates? If the remainder is non-zero there, instsimplify should fold this down to a true or false value. Start with that patch?

We're missing tests/proofs for the 'nuw' variants of the eq/ne patterns. Also, what happens if the mul has both nsw and nuw?



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1814-1816
+    bool ReduceSigned = Cmp.isSigned() && (*MulC != 0 && C != MinInt);
+    bool ReduceUnsigned = Cmp.isUnsigned() && *MulC != 0;
+    bool ReduceEquality = Cmp.isEquality() && (*MulC != 0 && C != MinInt);
----------------
We should always bail out of this function if MulC is 0 right at the top. That means instsimplify hasn't run yet on this expression, so we're wasting time on (and potentially obscuring) a pattern that can be completely eliminated.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1819
+      APInt RemVal = Mul->hasNoSignedWrap() ? C.srem(*MulC) : C.urem(*MulC);
+      APInt ReducedVal = Mul->hasNoSignedWrap() ? C.sdiv(*MulC) : C.udiv(*MulC);
+      if (RemVal == 0) {
----------------
Sink the creation of ReducedVal. We don't want to do that unless we know the transform will fire.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1821-1823
+        if (MulC->isNegative()) {
+          Pred = ICmpInst::getSwappedPredicate(Pred);
+        }
----------------
Don't use braces here according to LLVM coding guidelines.


Repository:
  rL LLVM

https://reviews.llvm.org/D52596