Skip to content

[Inliner] Don't propagate memory attributes to byval params #93381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 26, 2024
Merged

[Inliner] Don't propagate memory attributes to byval params #93381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions llvm/lib/Transforms/Utils/InlineFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could keep the original attributes and only drop if they are new?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think skipping byval in generaly makes sense. E.g. we also wouldn't want to propagate align or dereferenceable to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No gripe with this going in as is.

unsigned ArgNo = Arg->getArgNo();
// If so, propagate its access attributes.
AL = AL.addParamAttributes(Context, I, ValidParamAttrs[ArgNo]);
Expand Down
18 changes: 18 additions & 0 deletions llvm/test/Transforms/Inline/access-attributes-prop.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:%.*]]) {
Expand Down Expand Up @@ -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:%.*]]) {
Expand Down Expand Up @@ -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
}
Loading