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

[Balaram Makam via llvm-commits]

bmakam added inline comments.

================
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 =
----------------
sanjoy wrote:
> I think you also need to check that `TrueBB` and `FalseBB` are not equal.
I think we won't match this pattern if TrueBB == FalseBB because instcombine will already have transformed such IR into br i1 undef, label %dest, label %dest. I will add an assert to make sure they aren't equal.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3288
@@ +3287,3 @@
+              ? ConstantRange::makeAllowedICmpRegion(Pred, CI2->getValue())
+              : ConstantRange::makeAllowedICmpRegion(Pred, CI2->getValue())
+                    .inverse();
----------------
sanjoy wrote:
> 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.
I am not sure if I follow this correctly, could you please give an example?. 
If the new helper takes an APInt as RHS isn't it a singleton set in this case? 
Taking a general example if Pred is ult and Other is i8 [2, 5) makeAllowedICmpRegion would return [0, 4) and make SatisfyingICmpRegion would return [0, 2). Isn't a set that is equal to both same as the "satisfying" icmp range i.e [0, 2) in this example?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3290
@@ +3289,3 @@
+                    .inverse();
+      ConstantRange RHSRange = ConstantRange::makeAllowedICmpRegion(
+          I.getPredicate(), CI->getValue());
----------------
sanjoy wrote:
> 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`?
How about using CR and DominatingCR corresponding to CI and CI2?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3295
@@ +3294,3 @@
+        return replaceInstUsesWith(I, Builder->getFalse());
+      if (!isSignBit && !I.isEquality()) {
+        if (Intersection.isSingleElement()) {
----------------
sanjoy wrote:
> Why do you care about `isSignBit`?
If it is a SignBit, canonicalizing could transform for example an icmp slt 0 into icmp eq/ne 0 which we want to avoid in situations where the icmp is only used as input to a branch because it will pessimize the codegen by generating a compare and branch on zero instruction (cbz) instead of a test and branch(tbz).  tbz has better branch displacement than a cbz. If the icmp is an input to anything other than branches such as a select we would be better of canonicalizing it to eq/ne. Added a helper function isZBranchCheck to restrict it further for this case and added testcases.


http://reviews.llvm.org/D18841