[PATCH] D14263: [LVI] Clarify invariants, common code, and fix latent bugs

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Some comments inline.  Given that I'm not familiar with LVI, take the comments with a grain of salt.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:578
@@ +577,3 @@
+    // use them.
+    if (Res.isUndefined()) {
+      DEBUG(dbgs() << " compute BB '" << BB->getName()
----------------
I think the same logic can be better stated by having `Res` be a `Optional<LVILatticeVal>` instead of `LVILatticeVal`.  Using the `undefined` state this way is confusing as I'd normally assume a value to be `undefined` to mean that the code that uses it is dead.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1041
@@ +1040,3 @@
+  if (!hasBlockValue(V, BB)) {
+    pushBlockValue(std::make_pair(BB, V));
+    solve();
----------------
Does this mean the assert in `pushBlockValue` can be strengthened to `assert(!hasBlockValue(V, BB))` (instead of just checking for a `Constant`)?

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1061
@@ +1060,3 @@
+  if (BB->getTerminator() == CxtI)
+    Result = getValueInBlock(V, BB, CxtI);
+  else {
----------------
If `getValueInBlock` computes the result at the end of `BB`, is `CxtI` a redundant parameter?

================
Comment at: test/Transforms/CorrelatedValuePropagation/basic.ll:227
@@ +226,3 @@
+  call void @llvm.assume(i1 %res)
+  br i1 %res, label %taken, label %untaken
+taken:
----------------
> we can use assume facts to prove things about terminators

So CVP can simplify the `%res` use in the branch?  If so, can you please add a `CHECK` line checking for the same?


http://reviews.llvm.org/D14263