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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Hi Balaram,

I've replied to your inline comment, but it is possible that we're just talking past each other.  I'm usually on #llvm as `sanjoyd` on PDT daytime, we can chat there in realtime if you think that'll help.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3294
@@ +3293,3 @@
+                             TrueBB, FalseBB))) {
+      assert(TrueBB != FalseBB && "TrueBB must not be equal to FalseBB");
+      ConstantRange CR = ConstantRange::makeAllowedICmpRegion(I.getPredicate(),
----------------
Add a comment here on why that is true.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3297
@@ +3296,3 @@
+                                                              CI->getValue());
+      ConstantRange DominatingCR =
+          (Parent == TrueBB)
----------------
> I think this assumption is not always true, see my previous example.

Are you talking about the "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?" bit?  In that case isn't the RHS (i.e. `Other`)  `[2, 5)`, specifically //not// a singleton set?  If you're talking about another example, then I must have missed it.

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

That's not the only place where the current code will not generalize to a non-singleton RHS.  For instance, the `Intersection.isSingleElement()` check won't generalize either -- you cannot transform

```
if (a u< "[2, 5)") {
  // AllowedRegion = [0, 4)
  if (a >=u "[3, 5)") {
    // AllowedRegion = [3, 0)
    print();
  } else
    abort();
}
```

to

```
if (a u< "[2, 5)")
  if (a == 3)
    print();
  else
    abort();
```

since for `"[2, 5)"` == `4` and `"[3, 5)"` == `4` and `a` == `3`, the first program would have called `abort()` but the new program will call `print()`.

On the other hand, for the `Intersection.isEmptySet()` you do need to call `AllowedICmpRegion` on both the conditions.

There is a general pattern here -- `makeAllowedICmpRegion` only lets you "rule out" certain values (e.g. if `a >=u "[3, 5)"` then `a` definitely cannot be in {`0`, `1`, `2`}, but it //can// be anything else), but it does not guarantee that the comparison will fold one way or the other.  IOW, it is the "only if" part of the constraint (the comparison succeeds only if the LHS in the returned set), but not the "if" part (just because LHS is in the returned set does not mean the comparison will succeed).

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

Sure, but in that case we need to have the logic phrased in a way that makes it obvious that the current logic is not fully general (i.e. with a `makeExactICmpRegion`).

I don't think non-singletom RHS ranges are too rare:

```
if (a u< 100)
  if (b u< a)
    if (b >u 500)
```

In the inner condition `b u< a`, you know the RHS is in `[0, 100)`, and you can use that information to fold the innermost condition to `false`.  However, it is fine to not handle these cases as a starting point.


http://reviews.llvm.org/D18841