[PATCH] D60582: [IPSCCP] Add general integer range support.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Sorry about the delayed response.

So currently, this is essentially a three-patch series; D60581 <https://reviews.llvm.org/D60581>, D60582 <https://reviews.llvm.org/D60582>, and D61314 <https://reviews.llvm.org/D61314>.  D61314 <https://reviews.llvm.org/D61314> proposes to get rid of "forced" constants, and use plain constants instead.  I'm a little confused how that works; are you just getting rid of the assertion that a constant can't have two different values?

How do constant ranges work with loops?  It seems like under a naive implementation, an induction variable would start a 0, then grow to 0-1, then grow to 0-2, etc., until it covers the full integer range.  That clearly doesn't happen, since it would take forever, but I'm not sure I understand the limiting factor.  There's a comment in visitBinaryOperator about this, but does that cover all possible loops?

I'm a little wary of adding complexity to the lattice given the issues we currently have reasoning about the semantics of "undef"/ResolveUndefsIn.



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1867
   } else if (BranchInst *BI = dyn_cast<BranchInst>(I)) {
-    if (!isa<ConstantInt>(BI->getCondition())) {
+    if (BI->isConditional() && !isa<ConstantInt>(BI->getCondition())) {
       // Indeterminate branch; use false.
----------------
Does this change actually have an effect?  An unconditional branch should always flow to its successor.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2102
+      continue;
+    assert(isConstant(I->second) &&
            "Overdefined values should have been taken out of the map!");
----------------
This assertion makes no sense.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60582/new/

https://reviews.llvm.org/D60582