[llvm] goldsteinn/byval inline attr (PR #112256)

[via llvm-commits]

https://github.com/goldsteinn created https://github.com/llvm/llvm-project/pull/112256

- **[Inliner] Add tests for bad propagationg of access attr for `byval` param; NFC**
- **[Inliner] Don't propagate access attr to `byval` params**


>From 73e4a18ca240338d32740523e908af50c72b59a0 Mon Sep 17 00:00:00 2001
From: Noah Goldstein <goldstein.w.n at gmail.com>
Date: Mon, 14 Oct 2024 10:51:31 -0500
Subject: [PATCH 1/2] [Inliner] Add tests for bad propagationg of access attr
 for `byval` param; NFC

---
 .../Inline/access-attributes-prop.ll          | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/llvm/test/Transforms/Inline/access-attributes-prop.ll b/llvm/test/Transforms/Inline/access-attributes-prop.ll
index 2c55f5f3b1f6ca..1374b4bd3a9e4c 100644
--- a/llvm/test/Transforms/Inline/access-attributes-prop.ll
+++ b/llvm/test/Transforms/Inline/access-attributes-prop.ll
@@ -580,3 +580,23 @@ define ptr @callee_bad_param_prop(ptr readonly %x) {
   %r = tail call ptr @llvm.ptrmask(ptr %x, i64 -1)
   ret ptr %r
 }
+
+define dso_local void @foo_byval_readonly2(ptr readonly %p) {
+; CHECK-LABEL: define {{[^@]+}}@foo_byval_readonly2
+; CHECK-SAME: (ptr readonly [[P:%.*]]) {
+; CHECK-NEXT:    call void @bar4(ptr [[P]])
+; CHECK-NEXT:    ret void
+;
+  call void @bar4(ptr %p)
+  ret void
+}
+
+define void @prop_byval_readonly2(ptr %p) {
+; CHECK-LABEL: define {{[^@]+}}@prop_byval_readonly2
+; CHECK-SAME: (ptr [[P:%.*]]) {
+; CHECK-NEXT:    call void @bar4(ptr readonly [[P]])
+; CHECK-NEXT:    ret void
+;
+  call void @foo_byval_readonly2(ptr %p)
+  ret void
+}

>From ae11d1f70cd1432928aa4303531be5dc9b4e8bda Mon Sep 17 00:00:00 2001
From: Noah Goldstein <goldstein.w.n at gmail.com>
Date: Mon, 14 Oct 2024 10:51:35 -0500
Subject: [PATCH 2/2] [Inliner] Don't propagate access attr to `byval` params

We previously only handled the case where the `byval` attr was in the
callbase's param attr list. This PR also handles the case if the
`ByVal` was a param attr on the function's param attr list.
---
 llvm/lib/Transforms/Utils/InlineFunction.cpp    | 17 +++++++++++------
 .../Transforms/Inline/access-attributes-prop.ll |  2 +-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 110fd6de5c6968..c8a2fd95e3a970 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1398,7 +1398,7 @@ static void AddParamAndFnBasicAttributes(const CallBase &CB,
         if (!Arg)
           continue;
 
-        if (AL.hasParamAttr(I, Attribute::ByVal))
+        if (NewInnerCB->getParamAttr(I, Attribute::ByVal).isValid())
           // It's unsound to propagate memory attributes to byval arguments.
           // Even if CalledFunction doesn't e.g. write to the argument,
           // the call to NewInnerCB may write to its by-value copy.
@@ -1407,24 +1407,29 @@ static void AddParamAndFnBasicAttributes(const CallBase &CB,
         unsigned ArgNo = Arg->getArgNo();
         // If so, propagate its access attributes.
         AL = AL.addParamAttributes(Context, I, ValidParamAttrs[ArgNo]);
+        auto NewCBHasParamAttr = [&](Attribute::AttrKind Kind) {
+          return AL.hasParamAttr(I, Kind) ||
+                 NewInnerCB->getParamAttr(I, Kind).isValid();
+        };
+
         // We can have conflicting attributes from the inner callsite and
         // to-be-inlined callsite. In that case, choose the most
         // restrictive.
 
         // readonly + writeonly means we can never deref so make readnone.
-        if (AL.hasParamAttr(I, Attribute::ReadOnly) &&
-            AL.hasParamAttr(I, Attribute::WriteOnly))
+        if (NewCBHasParamAttr(Attribute::ReadOnly) &&
+            NewCBHasParamAttr(Attribute::WriteOnly))
           AL = AL.addParamAttribute(Context, I, Attribute::ReadNone);
 
         // If have readnone, need to clear readonly/writeonly
-        if (AL.hasParamAttr(I, Attribute::ReadNone)) {
+        if (NewCBHasParamAttr(Attribute::ReadNone)) {
           AL = AL.removeParamAttribute(Context, I, Attribute::ReadOnly);
           AL = AL.removeParamAttribute(Context, I, Attribute::WriteOnly);
         }
 
         // Writable cannot exist in conjunction w/ readonly/readnone
-        if (AL.hasParamAttr(I, Attribute::ReadOnly) ||
-            AL.hasParamAttr(I, Attribute::ReadNone))
+        if (NewCBHasParamAttr(Attribute::ReadOnly) ||
+            NewCBHasParamAttr(Attribute::ReadNone))
           AL = AL.removeParamAttribute(Context, I, Attribute::Writable);
       }
       NewInnerCB->setAttributes(AL);
diff --git a/llvm/test/Transforms/Inline/access-attributes-prop.ll b/llvm/test/Transforms/Inline/access-attributes-prop.ll
index 1374b4bd3a9e4c..5051c92345ec75 100644
--- a/llvm/test/Transforms/Inline/access-attributes-prop.ll
+++ b/llvm/test/Transforms/Inline/access-attributes-prop.ll
@@ -594,7 +594,7 @@ define dso_local void @foo_byval_readonly2(ptr readonly %p) {
 define void @prop_byval_readonly2(ptr %p) {
 ; CHECK-LABEL: define {{[^@]+}}@prop_byval_readonly2
 ; CHECK-SAME: (ptr [[P:%.*]]) {
-; CHECK-NEXT:    call void @bar4(ptr readonly [[P]])
+; CHECK-NEXT:    call void @bar4(ptr [[P]])
 ; CHECK-NEXT:    ret void
 ;
   call void @foo_byval_readonly2(ptr %p)