[PATCH] D69914: [LVI] Normalize pointer behavior

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

This is definitely heading in the right direction.  Nicely done overall, see suggestions as inline comments.



================
Comment at: llvm/lib/Analysis/LazyValueInfo.cpp:270
 
-void LazyValueInfoCache::eraseValue(Value *V) {
-  for (auto I = OverDefinedCache.begin(), E = OverDefinedCache.end(); I != E;) {
+void LazyValueInfoCache::eraseValueFromPerBlockValueCache(
+    Value *V, PerBlockValueCacheTy &Cache) {
----------------
Minor: I think this can become a free function as opposed to a member function.  It doesn't appear to be using member state.


================
Comment at: llvm/lib/Analysis/LazyValueInfo.cpp:689
+    Value *Ptr, SmallPtrSet<Value *, 4> &PtrSet, const DataLayout &DL) {
+  if (Ptr->getType()->getPointerAddressSpace() == 0) {
+    Ptr = GetUnderlyingObject(Ptr, DL);
----------------
Please add a todo about using address space property (i.e. null is defined) instead of the raw AS==0 check here.


================
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.
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69914