[PATCH] D14476: [LVI] Introduce an intersect operation on lattice values

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:991-995
@@ +990,7 @@
+///   explicitly promote undefined to overdefined before giving up.
+/// * Due to unreachable code, the intersection of two lattice values could be
+///   contradictory.  If this happens, we return some valid lattice value so as
+///   not confuse the rest of LVI.  We could introduce a special unreachable
+///   state into the lattice if we wanted to, but this doesn't seem likely to
+///   be worthwhile.
+static LVILatticeVal intersect(LVILatticeVal A, LVILatticeVal B) {
----------------
nlewycky wrote:
> Optional: You could return a single definition of the constant 'undef'?
I'm really trying to avoid changing fundamentals of the lattice with this change.  If you think this is a valuable enhancement given discussion below, let me know and I'll consider a future patch.  

================
Comment at: lib/Analysis/LazyValueInfo.cpp:997-1007
@@ +996,13 @@
+static LVILatticeVal intersect(LVILatticeVal A, LVILatticeVal B) {
+  // No new facts from either A or B, leave the other side in place
+  if (A.isUndefined())
+    return B;
+  if (B.isUndefined())
+    return A;
+
+  // If we gave up for one, but got a useable fact from the other, use it.
+  if (A.isOverdefined())
+    return B;
+  if (B.isOverdefined())
+    return A;
+
----------------
nlewycky wrote:
> I think that if it's undefined, that means that "there exists no definition for this value", and that's stronger than all other possibilities. I think if it's proven to be undefined by any means, you should return undefined.
I think the interpretation you're giving is a reasonable one, but it's not clear to me that's what is currently implemented in LVI.  I can't find a counter example, so I'm willing to do this, but would you mind if I submitted this change as is, then followed with a separate change to invert the handling for undefined and clarify the comments?  I'd like to isolate any breakage into an easy to revert patch.  I also think the diff will be fairly large without adding much value in understandability to the current patch.  

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1009-1010
@@ +1008,4 @@
+
+  // Can't get any more precise than constants (without explicitly modeling
+  // unreachable code)
+  if (hasSingleValue(A))
----------------
nlewycky wrote:
> Optional: Well, you can still rank constants. ConstantExpr is worse than ConstantInt is worse than Undef.
> 
> (Actually, there's something even more interesting here which is that constant A must equal constant B, which might be something nobody else has figured out yet. GVN would like to know.)
While interesting, this would be a fundamental change to the nature of the lattice.  We'd effectively be increasing it's height by the number of constant types.  Without a clear motivating example, I'd rather not go here.  

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1052
@@ -1001,2 +1051,3 @@
       return false;
-    Result.markOverdefined();
+    // No new information..
+    Result = LocalResult;
----------------
nlewycky wrote:
> One dot or three, never two.
Will fix.


http://reviews.llvm.org/D14476