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

[Philip Reames via llvm-commits]

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

Marking as request changes mostly to get it off my queue.  Feel free to reverse once you're ready for more discussion.


================
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.
----------------
FALSE - constantrange applies only to integers.  constant/notconstant apply only to non-integers.  Yes, that is very badly abstracted in the current code.  

================
Comment at: lib/Analysis/LazyValueInfo.cpp:209
@@ +208,3 @@
+    // undefined would be top and overdefined bottom. So in this
+    // case the function should return bottom in the case of isOverdefined, too.
+    // If both are undefined there should be no change.
----------------
True, but it does?  I don't see your point.  

================
Comment at: lib/Analysis/LazyValueInfo.cpp:210
@@ -201,1 +209,3 @@
+    // case the function should return bottom in the case of isOverdefined, too.
+    // If both are undefined there should be no change.
     if (RHS.isUndefined() || isOverdefined()) return false;
----------------
TRUE, but probably not useful in practice.  Meeting dead code is relatively rare.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:256
@@ -241,1 +255,3 @@
     if (isNotConstant()) {
+      // FIXME: how can isNotConstant.Val be equal to a constant???
+      // Example?
----------------
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.

================
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.
+
----------------
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.  

================
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:
----------------
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.  

================
Comment at: lib/Analysis/LazyValueInfo.cpp:861
@@ -814,1 +860,3 @@
     // the cache key in the caller.
+    // FIXME; Same as above:  When EdgesMissing is set, it  is never reset and
+    // this funciton
----------------
Same as above.

================
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:
----------------
Can you clarify your question?  This snippet seems to match the idiom used throughout based on a quick scan of the code.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1291
@@ -1232,1 +1290,3 @@
   assert(BlockValueStack.empty() && BlockValueSet.empty());
+  // FIXME: so this is the unusual pattern with hasBlockValue(). See
+  // the FIXME at one of the other instances. Why is solve() invoked
----------------
Because this is the outer interface which starts the recursive walk, not the inner step of the recursion.  If you want a more detailed explanation of the code, let me know.  I'll check something in which updates comments to make this more clear.


http://reviews.llvm.org/D19002