[PATCH] D18841: [InstCombine] Canonicalize icmp instructions based on dominating conditions.

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3284
@@ +3283,3 @@
+    if (BI && match(BI, m_Br(m_ICmp(Pred, m_Specific(Op0), m_ConstantInt(CI2)),
+                             TrueBB, FalseBB))) {
+      ConstantRange LHSRange =
----------------
I think you also need to check that `TrueBB` and `FalseBB` are not equal.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3288
@@ +3287,3 @@
+              ? ConstantRange::makeAllowedICmpRegion(Pred, CI2->getValue())
+              : ConstantRange::makeAllowedICmpRegion(Pred, CI2->getValue())
+                    .inverse();
----------------
I think what you want is `makeAllowedICmpRegion(getInversePredicate(Pred), CI2)`.  In this case (where `Other` is a singleton set) what you have is fine, but generally (i.e. if the RHS was not a constant integer, but a range) it is not.

Actually, it might be clearer to introduce a new helper: `ConstantRange::makeExactICmpRegion` that takes an `APInt` as RHS, and returns a set that is equal to both the "allowed" and "satisfying" icmp range.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3290
@@ +3289,3 @@
+                    .inverse();
+      ConstantRange RHSRange = ConstantRange::makeAllowedICmpRegion(
+          I.getPredicate(), CI->getValue());
----------------
This is confusingly named -- by "RHS", I'd normally understand `CI` or `CI2`; but what you mean here is the dominated LHS.  How about calling it just that: `DominatedLHSRange`?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3295
@@ +3294,3 @@
+        return replaceInstUsesWith(I, Builder->getFalse());
+      if (!isSignBit && !I.isEquality()) {
+        if (Intersection.isSingleElement()) {
----------------
Why do you care about `isSignBit`?

================
Comment at: test/Transforms/InstCombine/icmp.ll:2038
@@ +2037,3 @@
+; CHECK-LABEL: lor.rhs:
+; CHECK-NOT: [[B:%.*]] = icmp sgt i64 %a, 0
+; CHECK: [[C:%.*]] = icmp eq i64 %a, %b
----------------
Can you add a dominated check with, say, `10` or something just to demonstrate that we're general here?


http://reviews.llvm.org/D18841