[FunctionAttrs] Only check ArgMem effects when inferring argument attrs

[nikic]

When inferring readonly/writeonly on arguments, if the argument is passed to a call, we should only check the ArgMem effects implied by the call -- we don't care whether the call reads/writes non-arg memory (captured pointers are not relevant here, because they will abort the analysis entirely).

This also fixes a regression that was introduced when moving to MemoryEffects: The code was still checking the old WriteOnly attribute on functions, which no longer exists.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

When inferring readonly/writeonly on arguments, if the argument is passed to a call, we should only check the ArgMem effects implied by the call -- we don't care whether the call reads/writes non-arg memory (captured pointers are not relevant here, because they will abort the analysis entirely).

This also fixes a regression that was introduced when moving to MemoryEffects: The code was still checking the old WriteOnly attribute on functions, which no longer exists.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/IPO/FunctionAttrs.cpp (+4-3)
• (modified) llvm/test/Transforms/FunctionAttrs/writeonly.ll (+5-5)

diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index 95b3204d02beb81..c6891f72ad48d40 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -668,7 +668,8 @@ determinePointerAccessAttrs(Argument *A,
               Worklist.push_back(&UU);
       }
 
-      if (CB.doesNotAccessMemory())
+      ModRefInfo ArgMR = CB.getMemoryEffects().getModRef(IRMemLocation::ArgMem);
+      if (isNoModRef(ArgMR))
         continue;
 
       if (Function *F = CB.getCalledFunction())
@@ -683,9 +684,9 @@ determinePointerAccessAttrs(Argument *A,
       // invokes with operand bundles.
       if (CB.doesNotAccessMemory(UseIndex)) {
         /* nop */
-      } else if (CB.onlyReadsMemory() || CB.onlyReadsMemory(UseIndex)) {
+      } else if (!isModSet(ArgMR) || CB.onlyReadsMemory(UseIndex)) {
         IsRead = true;
-      } else if (CB.hasFnAttr(Attribute::WriteOnly) ||
+      } else if (!isRefSet(ArgMR) ||
                  CB.dataOperandHasImpliedAttr(UseIndex, Attribute::WriteOnly)) {
         IsWrite = true;
       } else {
diff --git a/llvm/test/Transforms/FunctionAttrs/writeonly.ll b/llvm/test/Transforms/FunctionAttrs/writeonly.ll
index 01c2139dbadf64d..8676d5284682c78 100644
--- a/llvm/test/Transforms/FunctionAttrs/writeonly.ll
+++ b/llvm/test/Transforms/FunctionAttrs/writeonly.ll
@@ -215,7 +215,7 @@ define void @direct2(ptr %p) {
 define void @direct2b(ptr %p) {
 ; FNATTRS: Function Attrs: memory(write)
 ; FNATTRS-LABEL: define {{[^@]+}}@direct2b
-; FNATTRS-SAME: (ptr nocapture [[P:%.*]]) #[[ATTR8]] {
+; FNATTRS-SAME: (ptr nocapture writeonly [[P:%.*]]) #[[ATTR8]] {
 ; FNATTRS-NEXT:    call void @direct2_callee(ptr nocapture [[P]])
 ; FNATTRS-NEXT:    ret void
 ;
@@ -304,7 +304,7 @@ define void @fptr_test2(ptr %p, ptr %f) {
 define void @fptr_test3(ptr %p, ptr %f) {
 ; FNATTRS: Function Attrs: memory(write)
 ; FNATTRS-LABEL: define {{[^@]+}}@fptr_test3
-; FNATTRS-SAME: (ptr nocapture [[P:%.*]], ptr nocapture readonly [[F:%.*]]) #[[ATTR8]] {
+; FNATTRS-SAME: (ptr nocapture writeonly [[P:%.*]], ptr nocapture readonly [[F:%.*]]) #[[ATTR8]] {
 ; FNATTRS-NEXT:    call void [[F]](ptr nocapture [[P]]) #[[ATTR8]]
 ; FNATTRS-NEXT:    ret void
 ;
@@ -320,7 +320,7 @@ define void @fptr_test3(ptr %p, ptr %f) {
 
 define void @test_argmem_none_callee(ptr %p) {
 ; FNATTRS-LABEL: define {{[^@]+}}@test_argmem_none_callee
-; FNATTRS-SAME: (ptr nocapture [[P:%.*]]) {
+; FNATTRS-SAME: (ptr nocapture readnone [[P:%.*]]) {
 ; FNATTRS-NEXT:    call void @direct1_callee(ptr nocapture [[P]]) #[[ATTR9:[0-9]+]]
 ; FNATTRS-NEXT:    ret void
 ;
@@ -335,7 +335,7 @@ define void @test_argmem_none_callee(ptr %p) {
 
 define void @test_argmem_read_callee(ptr %p) {
 ; FNATTRS-LABEL: define {{[^@]+}}@test_argmem_read_callee
-; FNATTRS-SAME: (ptr nocapture [[P:%.*]]) {
+; FNATTRS-SAME: (ptr nocapture readonly [[P:%.*]]) {
 ; FNATTRS-NEXT:    call void @direct1_callee(ptr nocapture [[P]]) #[[ATTR10:[0-9]+]]
 ; FNATTRS-NEXT:    ret void
 ;
@@ -350,7 +350,7 @@ define void @test_argmem_read_callee(ptr %p) {
 
 define void @test_argmem_write_callee(ptr %p) {
 ; FNATTRS-LABEL: define {{[^@]+}}@test_argmem_write_callee
-; FNATTRS-SAME: (ptr nocapture [[P:%.*]]) {
+; FNATTRS-SAME: (ptr nocapture writeonly [[P:%.*]]) {
 ; FNATTRS-NEXT:    call void @direct1_callee(ptr nocapture [[P]]) #[[ATTR11:[0-9]+]]
 ; FNATTRS-NEXT:    ret void
 ;

[fhahn]

LGTM, thanks!

Looks like this now infers more precise attributes than Attributor and also highlights an incorrect write only inference for the pointer argument used as function address in a write only call in llvm/test/Transforms/FunctionAttrs/writeonly.ll:313.

I'll file issues for those