[deref] Use readonly to infer global dereferenceability in a callee

[reames]

Summary

As described by code comments.

[nlopes]

llvm/lib/IR/Value.cpp

748

This is why I prefer atomic predicates. This if is duplicated to the code above.
If a function is readonly, doesNotFreeMemory() & hasNoSync() should return true. Otherwise we need to replicate this reasoning all over, which is going to end up with inconsistent semantics in different places.
So we are building some technical debt here..

EDIT: just checked Function.h and we have this:

bool doesNotFreeMemory() const {
   return onlyReadsMemory() || hasFnAttribute(Attribute::NoFree);
}

So I guess we only need to patch hasNoSync().

[reames]

llvm/lib/IR/Value.cpp

748

This is a style question, and one we disagree on. I personally strongly prefer working directly with attributes and querying them directly cleaner and safer. Our predicates are often not well defined, and the attributes have at least an attempt at a definition.

If we want to infer one attribute from another, we should either a) duplicate the inference rule into a helper function and use it, or b) actually infer it and add it to the IR.

[reames]

Refresh per reviewer preference for attribute handling.

Byval, etc... split to https://reviews.llvm.org/D99310.

[reames]

ping

[reames]

Realized the refactored code has a serious code smell. Not a bug per se, but not something I want to check in.

NoSync requires no coordination. ReadOnly simply implies this thread isn't initiating coordination. Another thread could be signaling, and this thread could be in a spin loop.

The only reason this isn't an active bug is that we consider all atomic ordered reads to also write.

I'm going to roll this back to the previous version.

[reames]

Roll back to previous version (undoing response to review comment) due to code smell described in last update.

I think this version should go in.

[reames]

Clarify comments to avoid (subtly misleading) reasoning about inferring nosync. While it happens to be true that atomic reads prevent readonly being inferred today, I'd rather not bake that assumption into the comment and we don't need the stronger inference anyway.

[reames]

(Thought of a counter example - of comment, not implementation)

(To flesh out later - read side check for signaled state with checkpoint mechanism solely in caller to avoid need for write while still validly syncronized.)

[reames]

Note for anyone following along.

Given the amount this review has managed to confuse *me*, I'm planning a change in direction here. (More of a detour really.) I'm going to pause this patch and focus on having the default pipeline infer nosync in the same set of conditions that attributor does. Only once that is done will I then return and justify why readonly is a valid precondition for both inferences. I don't *think* the end result will change, but I decided I need to do a better job of making the argument with each step easy to follow and justify.

[reames]

The effect of this will be entirely subsumed by D99749, and work following in that vein.

llvm/lib/IR/Value.cpp

if (isa<Constant>(V))
     return false;
 
   // Handle byval/byref/sret/inalloca/preallocated arguments.  The storage
   // lifetime is guaranteed to be longer than the callee's lifetime.
   if (auto *A = dyn_cast<Argument>(V))
     if (A->hasPointeeInMemoryValueAttr())
       return false;
 
   const Function *F = nullptr;
   if (auto *I = dyn_cast<Instruction>(V))
     F = I->getFunction();
   if (auto *A = dyn_cast<Argument>(V))
     F = A->getParent();
 
   if (!F)
     return true;
 
   // A pointer to an object in a function which neither frees, nor can arrange
   // for another thread to free on its behalf, can not be freed in the scope
   // of the function.
   if (F->doesNotFreeMemory() && F->hasNoSync())
     return false;
 
+  // Free is modeled as writing to the freed memory, so a readonly implies
+  // nofree.  Additionally, coordination with another thread to have it free
+  // on our behalf requires at least one write to shared memory, so readonly
+  // also implies that no other thread can free it (in a well defined manner).
+  // Subtly, this is weaker than implying nosync as the later would require
+  // us to prove this thread didn't read from shared memory either.
+  // (NOTE: This case is important in practice as we don't infer nosync, but
+  // do infer readonly.)
+  if (F->onlyReadsMemory())
+    return false;
+
   // With garbage collection, deallocation typically occurs solely at or after
   // safepoints.  If we're compiling for a collector which uses the
   // gc.statepoint infrastructure, safepoints aren't explicitly present
   // in the IR until after lowering from abstract to physical machine model.
   // The collector could chose to mix explicit deallocation and gc'd objects
   // which is why we need the explicit opt in on a per collector basis.
   if (!F->hasGC())
     return true;

llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll

 ; CHECK: %p
 define void @infer_func_attrs1(i32* dereferenceable(8) %p) nofree nosync {
   call void @mayfree()
   %v = load i32, i32* %p
   ret void
 }
 
 ; CHECK-LABEL: 'infer_func_attrs2'
-; GLOBAL: %p
-; POINT-NOT: %p
-; FIXME: Can be inferred from attributes
+; CHECK: %p
 define void @infer_func_attrs2(i32* dereferenceable(8) %p) readonly {
   call void @mayfree()
   %v = load i32, i32* %p
   ret void
 }
 
 ; CHECK-LABEL: 'infer_noalias1'
 ; GLOBAL: %p