[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:118
@@ -117,1 +117,3 @@
 
+/// isZBranchCheck - Given an exploded icmp instruction, return true if any
+/// use of this comparison is a branch on zero instruction.
----------------
sanjoy wrote:
> I'd use an architecture neutral name here, if possible.
Does 'isBranchOnSignBitCheck' sound good?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3293
@@ +3292,3 @@
+    if (BI && match(BI, m_Br(m_ICmp(Pred, m_Specific(Op0), m_ConstantInt(CI2)),
+                             TrueBB, FalseBB))) {
+      assert(TrueBB != FalseBB && "TrueBB must not be equal to FalseBB");
----------------
sanjoy wrote:
> My bias is that it is better to have these transforms be correct in isolation, and not rely on ordering across a large pass like -instcombine.  But if you're sure that the "branch on undef" optimization is guaranteed to kick in before this optimization (i.e. that borders around "obvious"), then feel free to add an assert instead of checking and bailing out.
Since instcombine visits the instruction in topological order, we will always visit the dominating branch before visting the icmp instruction and so it is guranteed that the branch condition is undef when the condition is irrelevant. 

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3297
@@ +3296,3 @@
+                                                              CI->getValue());
+      ConstantRange DominatingCR =
+          (Parent == TrueBB)
----------------
sanjoy wrote:
> Yeah, I wasn't as clear as I should have been.
> 
> The "problem" here is that the logic is structured in a way that will not generalize to RHS being a non-singleton range.  For instance, if we have
> 
> ```
>   if (lhs u< "[2, 5)") {
>     ...
>   } else {
>     // This is what you're trying to optimize
>     if (lhs u< 3)
>       print("hello");
>   }
> ```
> 
> then currently you will infer the range of `lhs` on the else branch to be `[0, 4).inverse()` == `[4, 0)`, and fold `lhs u< 3` to `false`.  But `lhs u< 3` can be true also, if the RHS (the value we know is in `[2, 5)`) was `2` and `lhs` was `2`.  In other words, in the above IR you know that in the "then" case `lhs` cannot be unsigned-greater than 4, but that does not mean that in the else case `lhs` *has to be* unsigned greater than 4.
> 
> Now I think this won't be an observable bug if RHS is always a singleton set, but I'd like to have some measures in place that make it clear what's going on.  There are two ways we can do that:
> 
>  - For a singleton RHS, I think (but please verify) `makeAllowedICmpRegion` and `makeSatisfyingICmpRegion` will return the same exact set, which contains _exactly_ the value that the predicate is true for.  If this assumption is true, my original suggestion was to add a `makeExactICmpRegion` method as:
> 
> ```
> makeExactICmpRegion(...) {
>   assert(makeAllowedICmpRegion(...) == makeSatisfyingICmpRegion(...));
>   return makeAllowedICmpRegion(...);
> }
> ```
> 
> and to use that here instead of `makeAllowedICmpRegion`.  It would take an `APInt` as RHS, and if someone comes along and tries to generalize this code to work on non-singleton RHSs, they'll immediately know that it isn't a 100% straightforward.
> 
>  - The second possibility is to generalize this logic to work with a non-singleton RHS range.  To do that, we can start by allowing the RHS to be a load instruction with `!range` metadata, and eventually move over to even more sophisticated analyses.
> 
> For a singleton RHS, I think (but please verify) makeAllowedICmpRegion and makeSatisfyingICmpRegion will return the same exact set, which contains _exactly_ the value that the predicate is true for.

I think this assumption is not always true, see my previous example.

> then currently you will infer the range of lhs on the else branch to be [0, 4).inverse() == [4, 0)

I see your point here, I have already modified this to 'makeAllowedICmpRegion(getInversePredicate(Pred), CI2)' based on your earlier suggestion.


> The second possibility is to generalize this logic to work with a non-singleton RHS range. 

I have no idea if non-singleton RHS ranges occur enough to be worthwhile to generalize. Do you mind if I defer this suggestion?












http://reviews.llvm.org/D18841