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

[reames]

Summary

As described by code comments.

[nlopes]

llvm/lib/IR/Value.cpp

758

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

758

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

static bool canBeFreed(const Value *V) {
   // *) An alloca is deallocated when the function returns
   if (isa<GlobalValue>(V) || isa<Constant>(V) || isa<AllocaInst>(V))
     return false;
 
   if (auto *A = dyn_cast<Argument>(V)) {
     // An argument to a function which neither frees, nor can arrange
     // for another thread to free on it's behalf, can not be freed in it's
     // defined scope.
-    // TODO: Generalize this case for noalias call results
+    // TODO: Generalize these two cases for noalias call results
     const Function *F = A->getParent();
     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 requires at
+    // least one write to shared memory, so readonly also implies nosync.  As
+    // such, this reduces to the case just above. (NOTE: This case is important
+    // in practice as we don't infer nosync, but do infer readonly.)
+    if (F->onlyReadsMemory())
+      return false;
+
+    // Handle byval/byref/sret/inalloca/preallocated arguments.  The storage
+    // lifetime is guaranteed to be longer than the callee's lifetime.
+    if (Type *ArgMemTy = A->getPointeeInMemoryValueType())
+      if (ArgMemTy->isSized())
+        return false;
+
     // If there's another copy of V which is freed, then the noalias fact
     // must not have held (i.e. UB).  Note that unlike the function attribute
     // equivalent, nofree on an argument implies another thread can't free it
     // on F's behalf either.
     if (A->hasNoFreeAttr() && A->hasNoAliasAttr())
       return false;
 
     // Freeing requires writing to the object.  If there were another written

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

 %struct.A = type { [8 x i8], [5 x i8] }
 @globalstruct = external global %struct.A
 
 @globalptr.align1 = external global i8, align 1
 @globalptr.align16 = external global i8, align 16
 
 ; Loads from sret arguments
 ; CHECK-LABEL: 'test_sret'
-; GLOBAL: %sret_gep{{.*}}(aligned)
-; POINT-NOT: %sret_gep{{.*}}(aligned)
+; CHECK: %sret_gep{{.*}}(aligned)
 ; CHECK-NOT: %sret_gep_outside
 define void @test_sret(%struct.A* sret(%struct.A) %result) {
   %sret_gep = getelementptr inbounds %struct.A, %struct.A* %result, i64 0, i32 1, i64 2
   load i8, i8* %sret_gep
 
   %sret_gep_outside = getelementptr %struct.A, %struct.A* %result, i64 0, i32 1, i64 7
   load i8, i8* %sret_gep_outside
   ret void
define void @global_allocationsize() {
 
   %outside_allocation = getelementptr inbounds %struct.A, %struct.A* @globalstruct, i64 0, i32 1, i64 10
   %load12 = load i8, i8* %outside_allocation
   ret void
 }
 
 ; Loads from byval arguments
 ; CHECK-LABEL: 'byval'
-; GLOBAL: %i8_byval{{.*}}(aligned)
-; POINT-NOT: %i8_byval{{.*}}(aligned)
-; CHECK-NOT: %byval_cast
-; GLOBAL: %byval_gep{{.*}}(aligned)
-; POINT-NOT: %byval_gep{{.*}}(aligned)
-; FIXME: Should hold in the point semantics case too
+; CHECK: %i8_byval{{.*}}(aligned)
+; CHECK-NOT: %bad_byval_cast
+; CHECK: %byval_gep{{.*}}(aligned)
+; CHECK: %good_byval_cast{{.*}}(unaligned)
 define void @byval(i8* byval(i8) %i8_byval,
-                        %struct.A* byval(%struct.A) %A_byval) {
   call void @mayfree()
-  %i8_byval_load = load i8, i8* %i8_byval
+  load i8, i8* %i8_byval
 
-  %byval_cast = bitcast i8* %i8_byval to i32*
-  %bad_byval_load = load i32, i32* %byval_cast
+  %bad_byval_cast = bitcast i8* %i8_byval to i32*
+  load i32, i32* %bad_byval_cast
 
   %byval_gep = getelementptr inbounds %struct.A, %struct.A* %A_byval, i64 0, i32 1, i64 2
   load i8, i8* %byval_gep
+  %good_byval_cast = bitcast %struct.A* %A_byval to i32*
+  load i32, i32* %good_byval_cast
+
   ret void
 }
 
 ; CHECK-LABEL: 'f_0'
 ; GLOBAL: %ptr = inttoptr i32 %val to i32*, !dereferenceable !0
 ; POINT-NOT: %ptr = inttoptr i32 %val to i32*, !dereferenceable !0
 define i32 @f_0(i32 %val) {
   %ptr = inttoptr i32 %val to i32*, !dereferenceable !0

 ; 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'
 ; CHECK: %p