[deref] No need to check nosync in addition to nofree

[mkazantsev]

Summary

The attribute nofree guarantees that a function cannot free memory
by any means, directly or indirectly. If it is specified, there is no need to
check nosync separately because it imposes extra limitations (for example,
a function can sync but never write or free memory).

This was already discussed in D99135, but the discussion doesn't explain
case of synching readonly/nofree function. Even if nosync is implied by
something else, we still should not rely on this attribute being explicitly
propagated before we came there.

[reames]

Change rejected. the nofree function attribute does explicitly allow freeing by another thread. See e.g. discussion here: https://reviews.llvm.org/D100141#2695360

I am really curious how you stumbled into this? There's only one currently enabled case which uses this codepath, and it has an explicit check for the GC abstract machine model which should cover your use case as I understand it?

[mkazantsev]

In D100551#2695408, @reames wrote:

Change rejected. the nofree function attribute does explicitly allow freeing by another thread. See e.g. discussion here: https://reviews.llvm.org/D100141#2695360

I am really curious how you stumbled into this? There's only one currently enabled case which uses this codepath, and it has an explicit check for the GC abstract machine model which should cover your use case as I understand it?

I am planning to use this checker for loop load PRE. In case of concurrent GC, this check is too restrictive because nosync is never inferrable in practice.

[reames]

In D100551#2697733, @mkazantsev wrote:

In D100551#2695408, @reames wrote:

I am really curious how you stumbled into this? There's only one currently enabled case which uses this codepath, and it has an explicit check for the GC abstract machine model which should cover your use case as I understand it?

I am planning to use this checker for loop load PRE. In case of concurrent GC, this check is too restrictive because nosync is never inferrable in practice.

Max, it might be worth talking offline about the specifics of your case.

In the abstract machine model, nosync should be inferrable in many cases. In the abstract machine model, you also don't need to do, and there's an existing check in code. (Which you might need to extend downstream.)

llvm/lib/IR/Value.cpp

bool Value::canBeFreed() const {
   if (auto *I = dyn_cast<Instruction>(this))
     F = I->getFunction();
   if (auto *A = dyn_cast<Argument>(this))
     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())
+  // A pointer to an object in a function which doesn't free memory, directly
+  // or indirectly (e.g. via creation of other threads that do it).
+  if (F->doesNotFreeMemory())
     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.

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

 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
+; POINT: %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