[PATCH] D69914: [LVI] Normalize pointer behavior

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM.  Very nice cleanup, thank you for doing this.



================
Comment at: llvm/lib/Analysis/LazyValueInfo.cpp:852
+  if (BBLV.isOverdefined()) {
+    // Check whether we're checking at the terminator, and the pointer has
+    // been dereferenced in this block.
----------------
nikic wrote:
> reames wrote:
> > Ok, I don't think this placement quite works.  The challenge is that your changing the point of the block scan from the furthest reached block during the backwards walk (entry, or point of overdefine) to instead scan the query source block.
> > 
> > I'd suggest adjusting the two call sites you remove to use the new caching logic, and then only do the scan if a) context is terminator, or b) context is not in same block (i.e. full block case)
> > The challenge is that your changing the point of the block scan from the furthest reached block during the backwards walk (entry, or point of overdefine) to instead scan the query source block.
> 
> I believe the behavior of the scan essentially stays the same: intersectAssume() is not only used to calculate the final result of the query, it is also invoked internally by LVI when computing edge values. So if you query a pointer in `BB2` with incoming edge from `BB1`, then the first thing we do is calculate the edge value `BB1 -> BB2`, at which point we will intersectAssume() with the terminator of BB1.
> 
> > I'd suggest adjusting the two call sites you remove to use the new caching logic, and then only do the scan if a) context is terminator, or b) context is not in same block (i.e. full block case)
> 
> We'd have to insert checks into getEdgeValue() (always) and getValueAt() / getValueInBlock() (if context is terminator), which are exactly the places where we already use intersectAssume().
You are correct.  I'd misremembered the code and hadn't checked the callers before commenting.  Objection withdrawn.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69914/new/

https://reviews.llvm.org/D69914