[IR] Only require nofree in canBeFreed()

[nikic]

After #195658 we only need the function to be nofree, as this also precludes freeing via synchronization now. Being nosync is no longer required.

[llvmorg-github-actions]

@llvm/pr-subscribers-llvm-ir

Author: Nikita Popov (nikic)

Changes

After #195658 we only need the function to be nofree, as this also precludes freeing via synchronization now. Being nosync is no longer required.

---

Full diff: https://github.com/llvm/llvm-project/pull/200191.diff

2 Files Affected:

• (modified) llvm/lib/IR/Value.cpp (+5-6)
• (modified) llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll (+2-4)

diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index 7246adf7ec651..074d42d728c95 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -834,13 +834,12 @@ bool Value::canBeFreed() const {
   if (auto *A = dyn_cast<Argument>(this)) {
     if (A->hasPointeeInMemoryValueAttr())
       return false;
-    // 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.  Note that this logic is restricted to memory
-    // allocations in existance before the call; a nofree function *is* allowed
-    // to free memory it allocated.
+    // A nofree function can not free (including via synchronization) any
+    // allocations that existed prior to the call, but may free allocations
+    // created inside the function. This logic is limited to argument pointers,
+    // as they definitely exist prior to the call.
     const Function *F = A->getParent();
-    if (F->doesNotFreeMemory() && F->hasNoSync())
+    if (F->doesNotFreeMemory())
       return false;
   }
 
diff --git a/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll b/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
index fc40c68624f2d..43f2c156b2ddc 100644
--- a/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
+++ b/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
@@ -248,16 +248,14 @@ define void @negative(ptr dereferenceable(8) %p) {
 
 ; CHECK-LABEL: 'infer_func_attrs1'
 ; CHECK: %p
-define void @infer_func_attrs1(ptr dereferenceable(8) %p) nofree nosync {
+define void @infer_func_attrs1(ptr dereferenceable(8) %p) nofree {
   call void @mayfree()
   %v = load i32, ptr %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(ptr dereferenceable(8) %p) readonly {
   call void @mayfree()
   %v = load i32, ptr %p

[llvmorg-github-actions]

@llvm/pr-subscribers-llvm-analysis

Author: Nikita Popov (nikic)

Changes

After #195658 we only need the function to be nofree, as this also precludes freeing via synchronization now. Being nosync is no longer required.

---

Full diff: https://github.com/llvm/llvm-project/pull/200191.diff

2 Files Affected:

• (modified) llvm/lib/IR/Value.cpp (+5-6)
• (modified) llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll (+2-4)

diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index 7246adf7ec651..074d42d728c95 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -834,13 +834,12 @@ bool Value::canBeFreed() const {
   if (auto *A = dyn_cast<Argument>(this)) {
     if (A->hasPointeeInMemoryValueAttr())
       return false;
-    // 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.  Note that this logic is restricted to memory
-    // allocations in existance before the call; a nofree function *is* allowed
-    // to free memory it allocated.
+    // A nofree function can not free (including via synchronization) any
+    // allocations that existed prior to the call, but may free allocations
+    // created inside the function. This logic is limited to argument pointers,
+    // as they definitely exist prior to the call.
     const Function *F = A->getParent();
-    if (F->doesNotFreeMemory() && F->hasNoSync())
+    if (F->doesNotFreeMemory())
       return false;
   }
 
diff --git a/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll b/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
index fc40c68624f2d..43f2c156b2ddc 100644
--- a/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
+++ b/llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
@@ -248,16 +248,14 @@ define void @negative(ptr dereferenceable(8) %p) {
 
 ; CHECK-LABEL: 'infer_func_attrs1'
 ; CHECK: %p
-define void @infer_func_attrs1(ptr dereferenceable(8) %p) nofree nosync {
+define void @infer_func_attrs1(ptr dereferenceable(8) %p) nofree {
   call void @mayfree()
   %v = load i32, ptr %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(ptr dereferenceable(8) %p) readonly {
   call void @mayfree()
   %v = load i32, ptr %p

[efriedma-quic]

LGTM

[antoniofrighetto]

LG. Should willNotFreeBetween() require an update too?

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Lines 718 to 721 in 76cdf60

	// Make sure the current function cannot arrange for another thread to free on
	// its behalf.
	if (!CtxI->getFunction()->hasNoSync())
	return false;

[nikic]

@antoniofrighetto Yes, see #199002 for some related work.

[antoniofrighetto]

@antoniofrighetto Yes, see #199002 for some related work.

Thanks, I missed that.