[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: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.
----------------
I'd use an architecture neutral name here, if possible.

================
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");
----------------
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.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3297
@@ +3296,3 @@
+                                                              CI->getValue());
+      ConstantRange DominatingCR =
+          (Parent == TrueBB)
----------------
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.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3299
@@ +3298,3 @@
+          (Parent == TrueBB)
+              ? ConstantRange::makeAllowedICmpRegion(Pred, CI2->getValue())
+              : ConstantRange::makeAllowedICmpRegion(
----------------
SGTM


http://reviews.llvm.org/D18841