[PATCH] D32135: [LVI Printer] Rely on the LVI analysis functions rather than the LVI cache

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1904
+  for (auto *BBSucc : successors(ParentBB))
+      printResult(BBSucc);
+
----------------
Is it legal to call `getValueInBlock(I, BB)` if `I` does not domiate `BB`?  If it isn't legal, then the code above will not do the right thing in cases like:

```
  br cond, L, R

L:
  %v = def
  br R

R:
```

since you'll end up asking `getValueInBlock(%v, R)` when `%v` does not dominate `R`.


================
Comment at: test/Analysis/LazyValueAnalysis/lvi-after-jumpthreading.ll:59
-; CHECK: br i1 %cnd, label %backedge, label %exit
-loop:
-  %iv = phi i32 [0, %entry], [%iv.next, %backedge]
----------------
anna wrote:
> I will add this test in a follow up functional change to LVI code itself: change LVI result and remove an assert that triggers when we call `getValueInBlock(%iv2.next, loop)`. This seems a valid call because `%iv2.next` is used in the basic block `loop`, which means it's already defined. When printing LVI for `iv2.next` at block `loop`, we currently fail on the assert in:
> ```
> bool LazyValueInfoImpl::solveBlockValueNonLocal(..){
> if (BB == &BB->getParent()->getEntryBlock()) {
>     assert(isa<Argument>(Val) && "Unknown live-in to the entry block");
> }
> ```
> The assert makes sense (call solveBlock for values in entry block works only iff it is defined in the entry block OR is a function argument). However, `LVI::solve` function for `(iv2.next, loop)` recurses over the predecessors of the `loop` block, and this includes the entry block.  
> 
> Note: We don't trigger this solve in the Jumpthreading or CVP usage of LVI. But it seems a perfectly reasonable solve to trigger. 
> 
I'm not sure if it is correct to say that `%iv2.next` is used in `%loop` -- a PHI node's use counts as a use at the end of the incoming block.



https://reviews.llvm.org/D32135