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

[Nick Lewycky via llvm-commits]

nlewycky added inline comments.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:351
@@ -350,1 +350,3 @@
 
+      DEBUG(dbgs () << "PUSH: " << *BV.second << " in " << BV.first->getName()
+                    << "\n");
----------------
Remove extra space after 'dbgs'

================
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) {
----------------
Optional: You could return a single definition of the constant 'undef'?

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

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

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


http://reviews.llvm.org/D14476