[PATCH] D99135: [deref] Implement initial set of inference rules for deref-at-point

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:741
+  // *) An alloca is deallocated when the function returns
+  if (isa<GlobalValue>(V) || isa<Constant>(V) || isa<AllocaInst>(V))
+    return false;
----------------
nlopes wrote:
> an Alloca can be killed with the lifetime intrinsics. Storing to an alloca after lifetime_end is UB.
Right, but how does that affect dereferenceability?  The memory is still dereferenceable.  The contents may be undefined, but the access won't fault.

p.s. Lifetime intrinsics are very badly specified, and are inconsistent across the optimizer - including in the existing deref code.  I really don't want them to be a blocker here.  


================
Comment at: llvm/lib/IR/Value.cpp:756
+    // on F's behalf either.
+    if (A->hasNoFreeAttr() && A->hasNoAliasAttr())
+      return false;
----------------
nlopes wrote:
> I don't understand this argument of why hasNoSync isn't needed.
> All argument attributes are with respect to what the function does, not the environment. It would be impossible to infer attributes if the environment was considered.
> 
> My understanding is that readonly implies nofree. Can't see why nofree would have those special semantics
The focus here isn't on nofree implying nosync.  It's noalias which implies nosync.  In order to for there to be a written (freed) copy of the pointer accessible in the other thread, the noalias fact must be false.  If so, that's already UB.


================
Comment at: llvm/lib/IR/Value.cpp:769
+      return false;
+  }
+
----------------
nlopes wrote:
> FWIW missing byval case (can't be freed).
Handled in follow up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99135