Skip to content

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

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 5 commits into from
Jan 13, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f51b0d
pre-commit tests
AlexMaclean Jan 11, 2025
5ebe920
[FunctionAttrs] Treat byval calls as only reading ptrs
AlexMaclean Jan 11, 2025
24f49f4
address comments
AlexMaclean Jan 12, 2025
8789015
update test
AlexMaclean Jan 13, 2025
89d7648
address comments
AlexMaclean Jan 13, 2025
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
5 changes: 5 additions & 0 deletions llvm/include/llvm/IR/InstrTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1675,6 +1675,11 @@ class CallBase : public Instruction {
// FIXME: Once this API is no longer duplicated in `CallSite`, rename this to
// better indicate that this may return a conservative answer.
bool doesNotCapture(unsigned OpNo) const {
// If the argument is passed byval, the callee does not have access to the
// original pointer and thus cannot capture it.
if (OpNo < arg_size() && isByValArgument(OpNo))
return true;

return dataOperandHasImpliedAttr(OpNo, Attribute::NoCapture);
}

Expand Down
4 changes: 1 addition & 3 deletions llvm/lib/Analysis/AliasAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,9 +636,7 @@ ModRefInfo AAResults::callCapturesBefore(const Instruction *I,
// Only look at the no-capture or byval pointer arguments. If this
// pointer were passed to arguments that were neither of these, then it
// couldn't be no-capture.
if (!(*CI)->getType()->isPointerTy() ||
(!Call->doesNotCapture(ArgNo) && ArgNo < Call->arg_size() &&
!Call->isByValArgument(ArgNo)))
if (!(*CI)->getType()->isPointerTy() || !Call->doesNotCapture(ArgNo))
continue;

AliasResult AR =
Expand Down
7 changes: 5 additions & 2 deletions llvm/lib/Transforms/IPO/FunctionAttrs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ 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);

Expand Down Expand Up @@ -890,11 +890,14 @@ determinePointerAccessAttrs(Argument *A,
// can participate in the speculation.
break;

const bool IsByVal =
CB.isArgOperand(U) && CB.isByValArgument(CB.getArgOperandNo(U));

// The accessors used on call site here do the right thing for calls and
// 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)) {
Expand Down
22 changes: 22 additions & 0 deletions llvm/test/Transforms/FunctionAttrs/readattrs.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 nocapture 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: {{.*}}
13 changes: 3 additions & 10 deletions llvm/test/Transforms/MemCpyOpt/memcpy-byval-forwarding-clobbers.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,15 @@ declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture)
declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture)
declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg)

; %a.2's lifetime ends before the call to @check. Cannot replace
; %a.1 with %a.2 in the call to @check.
; %a.2's lifetime ends before the call to @check. We must remove the call to
; @llvm.lifetime.end in order to replace %a.1 with %a.2 in the call to @check.
define i1 @alloca_forwarding_lifetime_end_clobber() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

; CHECK-LABEL: @alloca_forwarding_lifetime_end_clobber(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[A_1:%.*]] = alloca i64, align 8
; CHECK-NEXT: [[A_2:%.*]] = alloca i64, align 8
; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 8, ptr [[A_2]])
; CHECK-NEXT: call void @init(ptr sret(i64) align 8 [[A_2]])
; CHECK-NEXT: store i8 0, ptr [[A_2]], align 1
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr [[A_1]], ptr [[A_2]], i64 8, i1 false)
; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 8, ptr [[A_2]])
; CHECK-NEXT: [[CALL:%.*]] = call i1 @check(ptr byval(i64) align 8 [[A_1]])
; CHECK-NEXT: [[CALL:%.*]] = call i1 @check(ptr byval(i64) align 8 [[A_2]])
; CHECK-NEXT: ret i1 [[CALL]]
;
entry:
Expand Down Expand Up @@ -94,13 +90,10 @@ entry:
define i1 @alloca_forwarding_unrelated_call_noclobber() {
; CHECK-LABEL: @alloca_forwarding_unrelated_call_noclobber(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[A_1:%.*]] = alloca i64, align 8
; CHECK-NEXT: [[A_2:%.*]] = alloca i64, align 8
; CHECK-NEXT: [[A_3:%.*]] = alloca i64, align 8
; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 8, ptr [[A_2]])
; CHECK-NEXT: call void @init(ptr sret(i64) align 8 [[A_2]])
; CHECK-NEXT: store i8 0, ptr [[A_2]], align 1
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr [[A_1]], ptr [[A_2]], i64 8, i1 false)
; CHECK-NEXT: call void @clobber(ptr [[A_3]])
; CHECK-NEXT: [[CALL:%.*]] = call i1 @check(ptr byval(i64) align 8 [[A_2]])
; CHECK-NEXT: ret i1 [[CALL]]
Expand Down
Loading