[PATCH] D25075: [LazyValueInfo] Understand monotonically increasing loops and identity values

[Philip Reames via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Detailed comments inline.  This change is conceptually complicated even if a small textual change.  I'd suggest thinking about how to factor out complexity into separate changes.



================
Comment at: lib/Analysis/LazyValueInfo.cpp:935
+    // before.
+    if (PhiVal == PN)
+      EdgeResult = intersect(EdgeResult, Result);
----------------
I had this whole long response written on why this was wrong when I realized I misread what you had in the new draft.  :)

I believe this piece is correct.  Please separate it into it's own review with test cases.  

The complexity here is that Result is a *speculative* set of facts.  This is relying on the fact that any result computed (after the entire loop), must be correct for the phi input.  You're implicitly assuming that a later fact will force the lattice to a conservative value if needed.  Yep, that sounds reasonable.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:940
+    // produce a much more accurate lattice value than overdefined.
+    if ((EdgeResult.isOverdefined() ||
+         (EdgeResult.isConstantRange() &&
----------------
My (strongly) suggested factoring would be:
1) Extract the current loop as a helper function called solveBlockValuePHINodeImpl
2) Analyze the phi using your new logic in a helper function which returns a LatticeValue.
3) Call both helper functions exactly once and return the intersected results.  

Another possible structure would be:
1) Create the helper function mentioned in 2 above.  
2) Intersect it's results with the return value of getEdgeValue in the loop.  

The later will produce slightly more precise results than the former when we know a fact about the incoming value into the loop, but would otherwise loose all knowledge in the loop.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:950
+      if ((NSW || NUW) && !Inc->isNegative()) {
+        APInt Start = Result.isConstant()
+          ? cast<ConstantInt>(Result.getConstant())->getValue()
----------------
Using the result part was through the loop in this manner is *incorrect*.  It contains a speculative set of facts which may be later misproven.  

p.s. I could see an line of argument in which this might be safe for this particular use, but a) I'm not sure that's what you intended and b) if it is, it needs to be clearly documented.

p.p.s. I wrote this before revising my comment on the PhiVal == PN case.  I'm now reasonable sure you're doing this intentionally, but please make the argument in comment form.   :)

p.p.p.s. I don't believe there is any guarantee as to the order in which we visit values in a phi node.  In particular, we may not have visited the preheader yet...


================
Comment at: lib/Analysis/LazyValueInfo.cpp:952
+          ? cast<ConstantInt>(Result.getConstant())->getValue()
+          : Result.getConstantRange().getLower();
+        APInt End = NSW ? APInt::getSignedMaxValue(Start.getBitWidth())
----------------
This whole chain of logic can become ConstantRange::makeNoWrapRegion


================
Comment at: lib/Analysis/LazyValueInfo.cpp:960
+        // it is recomputed based on this new information.
+        TheCache.eraseValue(PhiVal);
+      }
----------------
Manually playing with the cache is a really dangerous thing to be doing and will need to be solidly justified.  Please implement a variant of this without this piece, then post a separate review for this with clear test cases motivating it.


Repository:
  rL LLVM

https://reviews.llvm.org/D25075