[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:357
@@ -356,1 +356,3 @@
 
+      DEBUG(dbgs () << "PUSH: " << *BV.second << " in " << BV.first->getName()
+                    << "\n");
----------------
Please fix the extra space after 'dbgs'

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1055-1058
@@ +1054,6 @@
+    return true;
+  if (Val.isConstantRange() &&
+      Val.getConstantRange().isSingleElement())
+    // Integer constants are single element ranges
+    return true;
+  return false;
----------------
Optional: I'd reorder this test first on the grounds that it should be much more likely.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1090
@@ +1089,3 @@
+
+  // Can't get any more precise than constants
+  if (hasSingleValue(A))
----------------
A period at the end please.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1105-1107
@@ +1104,5 @@
+    A.getConstantRange().intersectWith(B.getConstantRange());
+  // Note: An empty range is implicitly converted to overdefined internally.
+  // TODO: We could instead use Undefined here since we've proven a conflict
+  // and thus know this path must be unreachable. 
+  return LVILatticeVal::getRange(Range);
----------------
If using Undefined here doesn't work today, please file a PR and link to that PR from here. It has to be a bug, right?

================
Comment at: test/Transforms/CorrelatedValuePropagation/conflict.ll:2-3
@@ +1,4 @@
+; RUN: opt -correlated-propagation -S < %s | FileCheck %s
+; Checks that we don't crash on conflicting facts about a value 
+; (i.e. unreachable code)
+
----------------
Can you check anything positive about the results, that we did or didn't fold away branches or comparisons maybe?

================
Comment at: test/Transforms/JumpThreading/induction.ll:2
@@ +1,3 @@
+; RUN: opt -S -jump-threading < %s | FileCheck %s
+
+
----------------
Extra newline.

================
Comment at: test/Transforms/JumpThreading/induction.ll:13
@@ +12,3 @@
+  
+;; We can use an inductive argument to prove %iv is always positive
+  %cnd = icmp sge i32 %iv, 0
----------------
Could you add a CHECK line which shows that this happened? The presence of a branch to the backedge isn't entirely clear.


http://reviews.llvm.org/D14476