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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Hi,

I think this is an overtly specific solution.  Ideally we'd do
something that would generalize to cases like

  if (a >= 5)
    if (a < 6) {
      FOO;
    }

to

  if (a == 5) {
    FOO;
  }

as well.

IMO the right layering for all the predicate logic you have here is in
`ConstantRange`.  Just like we have `makeAllowedICmpRegion` and
`makeSatisfyingICmpRegion`, we should have a `makeExactICmpRegion`
where the RHS is not a `ConstantRange` but an `APInt` and have it
return *exactly* the set of values that satisfy the predicate (i.e. a
value satisfies the predicate *iff* it is in that set).  Then the
current algorithm could become:

  For a dominating icmp instruction "Var Pred RHS",
      compute R_Dom = makeAllowedICmpRegion(Pred, getRange(RHS));
  If the current icmp is of the form "Var Pred ConstRHS" then
      compute R_Self = makeExactICmpRegion(Pred, ConstRHS)
      if R_Self.intersect(R_Dom) is empty:
        replace current icmp with false
      if R_Self.intersect(R_Dom) has exactly one element E:
        replace current icmp with "Var == E"
      if R_Self.intersect(R_Dom).complement() has exact one element E:
        replace current icmp with "Var != E"


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3263
@@ +3262,3 @@
+          isSignBitCheck(Pred, CI2, TrueIfSigned)) {
+        if ((TrueIfSigned && Parent == FalseBB) ||
+            (!TrueIfSigned && Parent == TrueBB)) {
----------------
If you're going to restrict yourself to this case, I don't think you should depend on `DT`.  Instead you can just check that the only predecessor of `Parent` is `TrueBB` or `FalseBB` (depending on `TrueIfSigned`) and be done with it, since the only value of `IDom` you'll succeed with here is `Parent->getSinglePredecessor()`.



http://reviews.llvm.org/D18841