[Inliner] Don't propagate memory attributes to byval params #93381
Conversation
Memory restrictions for params to the inlined function do not apply to the copies logically made when that function further passes its own params as byval. In other words, imagine that @foo() calls @bar(ptr readonly %p) which in turn calls @baz(ptr byval("...") %p) (passing the same %p). This is fully legal - baz is allowed to modify its copy of the object referenced by %p because the argument is passed by value. However, when inlining @bar into @foo, we can't say that the callsite is now @baz(ptr readonly byval("...") %p), as this would mean that @baz is not allowed to modify it's copy of the object pointed to by %p. LangRef says: "The copy is considered to belong to the caller not the callee (for example, readonly functions should not write to byval parameters)". This fixes a miscompile introduced by PR llvm#89024 in a program in the Google codebase.
|
@llvm/pr-subscribers-llvm-transforms Author: Krzysztof Pszeniczny (amharc) ChangesMemory restrictions for params to the inlined function do not apply to the copies logically made when that function further passes its own params as byval. In other words, imagine that This fixes a miscompile introduced by PR #89024 in a program in the Google codebase. Full diff: https://github.com/llvm/llvm-project/pull/93381.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 82daaedaa0e81..7b846f2d2d72d 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1389,6 +1389,12 @@ static void AddParamAndFnBasicAttributes(const CallBase &CB,
if (!Arg)
continue;
+ if (AL.hasParamAttr(I, Attribute::ByVal))
+ // 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.
+ continue;
+
unsigned ArgNo = Arg->getArgNo();
// If so, propagate its access attributes.
AL = AL.addParamAttributes(Context, I, ValidParamAttrs[ArgNo]);
diff --git a/llvm/test/Transforms/Inline/access-attributes-prop.ll b/llvm/test/Transforms/Inline/access-attributes-prop.ll
index ffd31fbe8ae10..48665e9bbafd1 100644
--- a/llvm/test/Transforms/Inline/access-attributes-prop.ll
+++ b/llvm/test/Transforms/Inline/access-attributes-prop.ll
@@ -6,6 +6,7 @@
declare void @bar1(ptr %p)
declare void @bar2(ptr %p, ptr %p2)
declare void @bar3(ptr writable %p)
+declare void @bar4(ptr byval([4 x i32]) %p)
define dso_local void @foo1_rdonly(ptr readonly %p) {
; CHECK-LABEL: define {{[^@]+}}@foo1_rdonly
; CHECK-SAME: (ptr readonly [[P:%.*]]) {
@@ -186,6 +187,15 @@ define dso_local void @foo2_through_obj(ptr %p, ptr %p2) {
ret void
}
+define dso_local void @foo_byval_readonly(ptr readonly %p) {
+; CHECK-LABEL: define {{[^@]+}}@foo_byval_readonly
+; CHECK-SAME: (ptr readonly [[P:%.*]])
+; CHECK-NEXT: call void @bar4(ptr byval([4 x i32]) [[P]])
+; CHECK-NEXT: ret void
+ call void @bar4(ptr byval([4 x i32]) %p)
+ ret void
+}
+
define void @prop_param_func_decl(ptr %p) {
; CHECK-LABEL: define {{[^@]+}}@prop_param_func_decl
; CHECK-SAME: (ptr [[P:%.*]]) {
@@ -539,3 +549,11 @@ define void @prop_no_conflict_writable2(ptr %p) {
ret void
}
+define void @prop_byval_readonly(ptr %p) {
+; CHECK-LABEL: define {{[^@]+}}@prop_byval_readonly
+; CHECK-SAME: (ptr [[P:%.*]]) {
+; CHECK-NEXT: call void @bar4(ptr byval([4 x i32]) [[P]])
+; CHECK-NEXT: ret void
+ call void @foo_byval_readonly(ptr %p)
+ ret void
+}
|
|
Tagging @goldsteinn, @nikic, @nhaehnle (I don't have permissions to assign reviewers myself). |
| // Even if CalledFunction doesn't e.g. write to the argument, | ||
| // the call to NewInnerCB may write to its by-value copy. | ||
| continue; | ||
|
|
There was a problem hiding this comment.
I am slightly partial to implementing this by dropping memory attrs if we have ByVal at the end (similiar to how we readnone + readonly/writeonly conflicts). I have plans to expand this function and think it will be easier to scale we handle conflicts that way.
There was a problem hiding this comment.
This would work too, but would also drop readonly etc. from calls which already had readonly byval(...) before inlining - not sure how much this matters though.
There was a problem hiding this comment.
You could keep the original attributes and only drop if they are new?
There was a problem hiding this comment.
I think skipping byval in generaly makes sense. E.g. we also wouldn't want to propagate align or dereferenceable to it.
There was a problem hiding this comment.
I guess I feel like we will inevitably hit a case where we do want to propagate an attr to byval, and that point, this code will become alot more complex. Although I guess the other hand is having the list out each attr to cross out with byval is also fairly complex.
There was a problem hiding this comment.
No gripe with this going in as is.
|
Think this make sense but im not 100% sure of the semantics here. |
|
If you think this PR is good to be merged, I'd appreciate clicking the "merge" button for me - I don't have commit access (I used to have it in the SVN days, but didn't contribute to LLVM enough during the GitHub era to request it again...). Thanks! |
Memory restrictions for params to the inlined function do not apply to the copies logically made when that function further passes its own params as byval.
In other words, imagine that
@foo()calls@bar(ptr readonly %p)which in turn calls@baz(ptr byval("...") %p)(passing the same%p). This is fully legal -bazis allowed to modify its copy of the object referenced by%pbecause the argument is passed by value. However, when inlining@barinto@foo, we can't say that the callsite is now@baz(ptr readonly byval("...") %p), as this would mean that@bazis not allowed to modify it's copy of the object pointed to by%p. LangRef says: "The copy is considered to belong to the caller not the callee (for example, readonly functions should not write to byval parameters)".This fixes a miscompile introduced by PR #89024 in a program in the Google codebase.