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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1904
+  for (auto *BBSucc : successors(ParentBB))
+      printResult(BBSucc);
+
----------------
sanjoy wrote:
> 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`.
If `I` does not dominate `BB`, we fail on an unrelated assert (because we reach the entry basic block, where the only values whose LVI can be calculated are arguments and values defined in the block). It actually boils down to the implicit assumption that we can't calculate getValueInBlock if `I` does not dominate `BB`.
 I would think that we return `undefined` for the example you've above, rather than fail on the assert.

This is the same assertion failure for the `test2`.

I'll make this a stricter requirement, rather than depend on successors. 


================
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]
----------------
sanjoy wrote:
> 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.
> 
Agreed. Phi-nodes need to be treated differently.


https://reviews.llvm.org/D32135