[PATCH] D19002: [LazyValueInfo] Fix for a nasty compile-time problem with questions

[Gerolf Hoflehner via llvm-commits]

Gerolf added a comment.

This should also take care of  http://reviews.llvm.org/D18066.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:64
@@ +63,3 @@
+/// FIXME:
+//  - constant and constantrange could be unified. A constant is only
+/// a special case of a constant range.
----------------
reames wrote:
> FALSE - constantrange applies only to integers.  constant/notconstant apply only to non-integers.  Yes, that is very badly abstracted in the current code.  
Ok, added a note with that comment instead.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:256
@@ -241,1 +255,3 @@
     if (isNotConstant()) {
+      // FIXME: how can isNotConstant.Val be equal to a constant???
+      // Example?
----------------
reames wrote:
> We have proven conflicting paths along two paths.  The is actually proving that a particular codepath is unreachable.  We could make this as undefined, but the overdefined result is conservatively correct.
Got it, thanks. The value should  bottom (overdefined) in this case, so that is fine.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:780
@@ +779,3 @@
+  // for that ~150K line funciton from "inifinite" to a couple of minutes.
+  // The rational is that "not null" is the best we can do for a stackaddress.
+
----------------
reames wrote:
> Your change here is reasonable, but your reasoning scares me.  What makes allocas special in this example?  Just that we have a lot of them?  Would the same test case with a bunch of globals be equally bad?
> 
> p.s. We can actually prove constant/not-constant facts about pointers.  This is useful when simplifying code involving equality checks between allocas and addressed obtained from globals.  Given this, I'm not entirely convinced this is a good idea.  I'm open to debating the compile time/precision tradeoff here, but it is a tradeoff which needs to be acknowledged and documented.  
"scares me".  I think the crux is that values are pushed on the stack while the solver is running (see the call chain solve -> etc above) . This is not the standard way how const prop/range solvers work and where the monotonicity assumption is violated. I believe but have no proof that the alloca's are only exposing the deeper problem. What we eventually will need here is a standard (forward) data flow solver that will prevent such problems. 

================
Comment at: lib/Analysis/LazyValueInfo.cpp:810
@@ -770,1 +809,3 @@
     EdgesMissing |= !getEdgeValue(Val, *PI, BB, EdgeResult);
+    // FIXME; When EdgesMissing is set, it  is never reset and this funciton
+    // will return false. Should it be:
----------------
reames wrote:
> You have missed a key detail of LVI.  "false" means go do more recurse work, and recall this function when another edge might be available.  This code is checking to see if any known input forces us to overdefined to avoid recursing in that common case.  
Say you have three edges. Assume getEdgeValue() returns false, overdefined , false. The result of the function is false. Assume getEdgeValue returns overdefined on the first edge. The result of the function is true, independent of the other edges. That doesn't look right. The order of the edges should not impact the result. I think this is the intent:
walk all edges. 
if any edge it overdefined: BBLV= overdefined. Return true.
Otherwise: merge all values. If any edge is missing return false else BBLV = merged value. return true.

But this is not what is implemented (and my code proposal didn't do any better here, so forget about it).

================
Comment at: lib/Analysis/LazyValueInfo.cpp:926
@@ -872,1 +925,3 @@
   // Recurse on our inputs if needed
+  // FIXME: look at all instances on hasBlockValue. All but one have this
+  // pattern:
----------------
reames wrote:
> Can you clarify your question?  This snippet seems to match the idiom used throughout based on a quick scan of the code.
Ok, this is the example you answered below.


http://reviews.llvm.org/D19002