[FunctionAttrs] Treat byval calls as only reading ptrs #122618

AlexMaclean · 2025-01-11T22:54:00Z

Since byval arguments are passed via a hidden copy of the pointee, they do not have the same semantics as normal pointer arguments. The callee cannot capture or write to the pointer and the copy is a read of the pointer.

llvmbot · 2025-01-11T22:54:31Z

@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Alex MacLean (AlexMaclean)

Changes

Since byval arguments are passed via a hidden copy of the pointee, they do not have the same semantics as normal pointer arguments. The callee cannot capture or write to the pointer and the copy is a read of the pointer.

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

2 Files Affected:

(modified) llvm/lib/Transforms/IPO/FunctionAttrs.cpp (+4-3)
(modified) llvm/test/Transforms/FunctionAttrs/readattrs.ll (+22)

diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index fe9cca01a8f31f..7ac9b70f47902c 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -852,9 +852,10 @@ determinePointerAccessAttrs(Argument *A,
         continue;
       }
 
-      // Given we've explictily handled the callee operand above, what's left
+      // Given we've explicitly handled the callee operand above, what's left
       // must be a data operand (e.g. argument or operand bundle)
       const unsigned UseIndex = CB.getDataOperandNo(U);
+      const bool IsByVal = CB.isByValArgument(CB.getArgOperandNo(U));
 
       // Some intrinsics (for instance ptrmask) do not capture their results,
       // but return results thas alias their pointer argument, and thus should
@@ -864,7 +865,7 @@ determinePointerAccessAttrs(Argument *A,
         for (Use &UU : CB.uses())
           if (Visited.insert(&UU).second)
             Worklist.push_back(&UU);
-      } else if (!CB.doesNotCapture(UseIndex)) {
+      } else if (!(CB.doesNotCapture(UseIndex) || IsByVal)) {
         if (!CB.onlyReadsMemory())
           // If the callee can save a copy into other memory, then simply
           // scanning uses of the call is insufficient.  We have no way
@@ -894,7 +895,7 @@ determinePointerAccessAttrs(Argument *A,
       // invokes with operand bundles.
       if (CB.doesNotAccessMemory(UseIndex)) {
         /* nop */
-      } else if (!isModSet(ArgMR) || CB.onlyReadsMemory(UseIndex)) {
+      } else if (!isModSet(ArgMR) || CB.onlyReadsMemory(UseIndex) || IsByVal) {
         IsRead = true;
       } else if (!isRefSet(ArgMR) ||
                  CB.dataOperandHasImpliedAttr(UseIndex, Attribute::WriteOnly)) {
diff --git a/llvm/test/Transforms/FunctionAttrs/readattrs.ll b/llvm/test/Transforms/FunctionAttrs/readattrs.ll
index 004c0485d764ae..6087160d13ca1d 100644
--- a/llvm/test/Transforms/FunctionAttrs/readattrs.ll
+++ b/llvm/test/Transforms/FunctionAttrs/readattrs.ll
@@ -762,5 +762,27 @@ define void @writable_readnone(ptr writable dereferenceable(4) %p) {
   ret void
 }
 
+declare void @byval_param(ptr byval(i32) %p)
+
+define void @call_byval_param(ptr %p) {
+; FNATTRS-LABEL: define {{[^@]+}}@call_byval_param
+; FNATTRS-SAME: (ptr readonly [[P:%.*]]) {
+; FNATTRS-NEXT:    call void @byval_param(ptr byval(i32) [[P]])
+; FNATTRS-NEXT:    ret void
+;
+; ATTRIBUTOR-LABEL: define {{[^@]+}}@call_byval_param
+; ATTRIBUTOR-SAME: (ptr nocapture readonly [[P:%.*]]) {
+; ATTRIBUTOR-NEXT:    call void @byval_param(ptr nocapture readonly byval(i32) [[P]])
+; ATTRIBUTOR-NEXT:    ret void
+;
+; ATTRIBUTOR-CGSCC-LABEL: define {{[^@]+}}@call_byval_param
+; ATTRIBUTOR-CGSCC-SAME: (ptr nocapture readonly [[P:%.*]]) {
+; ATTRIBUTOR-CGSCC-NEXT:    call void @byval_param(ptr nocapture readonly byval(i32) [[P]])
+; ATTRIBUTOR-CGSCC-NEXT:    ret void
+;
+  call void @byval_param(ptr byval(i32) %p)
+  ret void
+}
+
 ;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
 ; COMMON: {{.*}}

nikic

This basically looks right, but I think at least for doesNotCapture we'd be better off changing the API to automatically check byval. I looked at some of the other callers and many of them don't handle byval either, but should. (You can see in the test that FunctionAttrs infers readonly after your change, but still misses nocapture. That's because CaptureTracking doesn't check for byval.)

AlexMaclean · 2025-01-12T23:46:02Z

I think at least for doesNotCapture we'd be better off changing the API to automatically check byval.

Done, I now see nocapture added in the testcase as well.

@nikic do you think it makes sense to update CallBase::onlyReadsMemory with a similar check, or should I keep things as is?

nikic

Can you please drop the separate byval check in

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

Line 640 in 99612a3

(!Call->doesNotCapture(ArgNo) && ArgNo < Call->arg_size() &&

? It shouldn't be needed anymore.

nikic · 2025-01-13T14:54:15Z

llvm/test/Transforms/MemCpyOpt/memcpy-byval-forwarding-clobbers.ll

@@ -16,14 +16,10 @@ declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias
 define i1 @alloca_forwarding_lifetime_end_clobber() {


Should update the comment above here, because we now do the optimization by dropping the lifetime intrinsics.

nikic · 2025-01-13T14:57:33Z

@nikic do you think it makes sense to update CallBase::onlyReadsMemory with a similar check, or should I keep things as is?

I'm a bit less sure on that one. byval readonly on a function definition does mean something, which is that the byval copy is not read by the function. But from an external perspective, when analyzing a call-site, this distinction is not relevant. So overall I think it would make sense to change CallBase::onlyReadsMemory as well. But maybe as a followup PR?

AlexMaclean · 2025-01-13T16:31:32Z

Can you please drop the separate byval check in

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

Line 640 in 99612a3

(!Call->doesNotCapture(ArgNo) && ArgNo < Call->arg_size() &&

? It shouldn't be needed anymore.

Fixed!

But maybe as a followup PR?

Sounds good, I will take this up in a separate PR

nikic

LGTM

llvm-ci · 2025-01-14T00:13:13Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-expensive-checks-debian running on gribozavr4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/11907

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: tools/llvm-gsymutil/ARM_AArch64/macho-merged-funcs-dwarf.yaml' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
Input file: /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-gsymutil/ARM_AArch64/Output/macho-merged-funcs-dwarf.yaml.tmp.dSYM
Output file (aarch64): /b/1/llvm-clang-x86_64-expensive-checks-debian/build/test/tools/llvm-gsymutil/ARM_AArch64/Output/macho-merged-funcs-dwarf.yaml.tmp.default.gSYM
Loaded 3 functions from DWARF.
Loaded 3 functions from symbol table.
warning: same address range contains different debug info. Removing:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000047
addr=0x0000000000000248, file=  3, line=  5
addr=0x0000000000000254, file=  3, line=  7
addr=0x0000000000000258, file=  3, line=  9
addr=0x000000000000025c, file=  3, line=  8
addr=0x0000000000000260, file=  3, line= 11
addr=0x0000000000000264, file=  3, line= 10
addr=0x0000000000000268, file=  3, line=  6


In favor of this one:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000030
addr=0x0000000000000248, file=  2, line=  5
addr=0x0000000000000254, file=  2, line=  7
addr=0x0000000000000258, file=  2, line=  9
addr=0x000000000000025c, file=  2, line=  8
addr=0x0000000000000260, file=  2, line= 11
addr=0x0000000000000264, file=  2, line= 10
addr=0x0000000000000268, file=  2, line=  6


warning: same address range contains different debug info. Removing:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000030
addr=0x0000000000000248, file=  2, line=  5
addr=0x0000000000000254, file=  2, line=  7
addr=0x0000000000000258, file=  2, line=  9
addr=0x000000000000025c, file=  2, line=  8
addr=0x0000000000000260, file=  2, line= 11
addr=0x0000000000000264, file=  2, line= 10
addr=0x0000000000000268, file=  2, line=  6


In favor of this one:
[0x0000000000000248 - 0x0000000000000270): Name=0x00000001
addr=0x0000000000000248, file=  1, line=  5
addr=0x0000000000000254, file=  1, line=  7
addr=0x0000000000000258, file=  1, line=  9
addr=0x000000000000025c, file=  1, line=  8
addr=0x0000000000000260, file=  1, line= 11
addr=0x0000000000000264, file=  1, line= 10
...

pre-commit tests

8f51b0d

AlexMaclean requested review from nikic and krzysz00 January 11, 2025 22:54

AlexMaclean self-assigned this Jan 11, 2025

llvmbot added the llvm:transforms label Jan 11, 2025

[FunctionAttrs] Treat byval calls as only reading ptrs

5ebe920

AlexMaclean force-pushed the dev/amaclean/upstream/fun-attrs-byval-readonly branch from ddc39c5 to 5ebe920 Compare January 12, 2025 19:33

nikic reviewed Jan 12, 2025

View reviewed changes

address comments

24f49f4

llvmbot added the llvm:ir label Jan 12, 2025

update test

8789015

AlexMaclean requested a review from fhahn January 13, 2025 00:45

nikic reviewed Jan 13, 2025

View reviewed changes

address comments

89d7648

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jan 13, 2025

nikic approved these changes Jan 13, 2025

View reviewed changes

AlexMaclean merged commit c6c864d into llvm:main Jan 13, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FunctionAttrs] Treat byval calls as only reading ptrs #122618

[FunctionAttrs] Treat byval calls as only reading ptrs #122618

Uh oh!

AlexMaclean commented Jan 11, 2025

Uh oh!

llvmbot commented Jan 11, 2025 •

edited

Loading

Uh oh!

nikic left a comment

Uh oh!

AlexMaclean commented Jan 12, 2025

Uh oh!

nikic left a comment

Uh oh!

nikic Jan 13, 2025

Uh oh!

AlexMaclean Jan 13, 2025

Uh oh!

nikic commented Jan 13, 2025

Uh oh!

AlexMaclean commented Jan 13, 2025

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

llvm-ci commented Jan 14, 2025

Uh oh!

Uh oh!

		@@ -16,14 +16,10 @@ declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias
		define i1 @alloca_forwarding_lifetime_end_clobber() {

[FunctionAttrs] Treat byval calls as only reading ptrs #122618

[FunctionAttrs] Treat byval calls as only reading ptrs #122618

Uh oh!

Conversation

AlexMaclean commented Jan 11, 2025

Uh oh!

llvmbot commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

AlexMaclean commented Jan 12, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

nikic Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

AlexMaclean Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

nikic commented Jan 13, 2025

Uh oh!

AlexMaclean commented Jan 13, 2025

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Jan 14, 2025

Uh oh!

Uh oh!

llvmbot commented Jan 11, 2025 •

edited

Loading