[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:
> reames wrote:
> > reames wrote:
> > > 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.  
> > JFYI, the wording in the LangRef on semantics of a dead object (after lifetime end) was recently changed in c821ef4.  I objected to the wording at the time, though the patch appears not to have been adjusted.  This is why.  
> > 
> > lifetime intrinsics do not change dereferenceability.  The relevant wording from c821ef4 needs removed from the LangRef.  It is wrong.  Both with current and proposed semantics.
> I agree that doing a store after lifetime_end intrinsic doesn't cause a crash (ATM), but it may write to another object.
> For example:
>   %p = alloca i8
>   %q = alloca i8
>   
>   lifetime_start %p
>   ...
>   lifetime_end %p
>   lifetime_start %q
>   store i8 1, %q
>   store i8 0, %p ; UB: %p is dead
>   
>   %v = load i8, %q
> 
> What's the value of %v? In practice, it's going to be 0 because %p & %q will end up allocated at the same address. And this is why storing after lifetime_end must be UB.
> 
> So now it depends on how this analysis is used. My concern is that it may be used to move a store past lifetime_end and then we have a miscompilation. Because if a pointer is dereferenceable for the size of the store and it's guaranteed to be alive throughout the function, then  we can conclude that loads & stores to that pointer can be moved freely.
> 
> A simple solution is to check if the particular alloca is an argument to a lifetime_end intrinsic. More complex ones require an additional argument like a BB or specific instruction, such that the query becomes: is the object still alive at this BB/instruction?
This has turned into a discussion of the lifetime intrinsics.  I assert that is unrelated to this change (as the implementation here exactly matches the existing deref implementation for this case).  I ask that this discussion be removed from this review thread and taken elsewhere.

I'll also note that you are commingling concerns in your discussion.  Dereferenceability does NOT imply it's safe to insert a store.  That's a higher level property with additional concerns (e.g. concurrency) which need to be satisfied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99135