Skip to content

Inline: Fix handling of byval using non-alloca addrspace #97306

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 3 commits into from
Jul 1, 2024
Merged

Inline: Fix handling of byval using non-alloca addrspace #97306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77ceba2
Inline: Fix handling of byval using non-alloca addrspace
arsenm Jul 1, 2024
7fa34cd
Update InlineFunction.cpp
arsenm Jul 1, 2024
4cc0aa4
Run clang-format
arsenm Jul 1, 2024
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: 3 additions & 2 deletions llvm/lib/Transforms/Utils/InlineFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1674,8 +1674,9 @@ static Value *HandleByValArgument(Type *ByValType, Value *Arg,
if (ByValAlignment)
Alignment = std::max(Alignment, *ByValAlignment);

AllocaInst *NewAlloca = new AllocaInst(ByValType, DL.getAllocaAddrSpace(),
Copy link
Contributor

@MrSidims MrSidims Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@arsenm
Digging a bit into git log the patch that added DL.getAllocaAddrSpace() was the following:

commit 3c1fc768ed6cf2b01463df036ae6ae3b1f0de632
Author: Matt Arsenault <[email protected]>
Date:   Mon Apr 10 22:27:50 2017 +0000

    Allow DataLayout to specify addrspace for allocas.

and reasoning behind that patch:
" These are problems for AMDGPU because alloca is used to
implement the private address space, which uses a 32-bit
index as the pointer value. Other pointers are 64-bit
and behave more like LLVM's notion of generic address
space. By changing the address space used for allocas,
we can change our generic pointer type to be LLVM's generic
pointer type which does have similar properties."

makes sense to me especially in OpenCL context, where default address space is 4 (generic). Also IMHO LLVM passes must respect target's datalayout when picking possible types for the created instructions.

Current fix brings some complications though (from OpenCL point of view, but there might be issues for other languages and targets outside of my expertise), if byval pointer of a function was in default aka genetic address space, then the alloca would be in generic. Meanwhile per SPIR-V (to which LLVM IR is being lowered starting from OpenCL 2.0) specification OpVariable must have non-generic storage class. It can probably be workarounded in https://github.com/KhronosGroup/SPIRV-LLVM-Translator by translating alloca addrspace(4) to OpVariable with Private storage class bringing extra address space casts, or by modifying clang, introducing an exception for byval pointers making them not be in generic address space, but in private, but I wonder if #97086 can be solved differently - instead here if Arg->getType()->getPointerAddressSpace() != DL.getAllocaAddrSpace() - we still create an alloca in default datalayout address space and add an AS cast, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

instead here if Arg->getType()->getPointerAddressSpace() != DL.getAllocaAddrSpace() - we still create an alloca in default datalayout address space and add an AS cast, WDYT?

I'm not sure I fully understand the proposal, but for the CHERI use case (the AMD version was based on the CHERI fork, which we aim to start upstreaming soon), allocas must be created in the alloca address space, not some other address space and cast.

Copy link
Contributor

@MrSidims MrSidims Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I mean: leave alloca in DL.getAllocaAddrSpace() AS and if it doesn't match with byval pointer's address space - create a cast from alloca to byval's address space and update users of the alloca to use the cast. But digging more into it might be not as trivial as I though initially - at least it seems it shouldn't be done in HandleByValArgument, but probably later in the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would not break anything for CHERI, if it works for other use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is the correct fix. We should follow what the existing IR says. The rule is not all allocas must have the alloca address space, it's the alloca address space is the default for contextless new values.

As far as OpenCL is concerned such IR would have been malformed in the first place. If we wanted to handle this as a generic pointer with a private backed alloca, it should be handled by lowering the alloca with the wrong address space to include the cast (which is essentially what NVPTX does, it's just more work for the backend and less optimizable IR)

nullptr, Alignment, Arg->getName());
AllocaInst *NewAlloca =
new AllocaInst(ByValType, Arg->getType()->getPointerAddressSpace(),
nullptr, Alignment, Arg->getName());
NewAlloca->insertBefore(Caller->begin()->begin());
IFI.StaticAllocas.push_back(NewAlloca);

Expand Down
38 changes: 38 additions & 0 deletions llvm/test/Transforms/Inline/byval-with-non-alloca-addrspace.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt -S -passes=always-inline %s | FileCheck %s

; The byval parameters use a different address space from the alloca
; address space. Make sure this is gracefully handled by using the
; original byval pointer type's address space instead of what the
; datalayout says to use.

target datalayout = "A5"

%struct = type { i64, i64 }

define i64 @bar(ptr byval(%struct) %a) alwaysinline {
; CHECK-LABEL: define i64 @bar(
; CHECK-SAME: ptr byval([[STRUCT:%.*]]) [[A:%.*]]) #[[ATTR0:[0-9]+]] {
; CHECK-NEXT: [[TMP1:%.*]] = getelementptr [[STRUCT]], ptr [[A]], i64 0, i32 1
; CHECK-NEXT: [[TMP2:%.*]] = load i64, ptr [[TMP1]], align 4
; CHECK-NEXT: ret i64 0
;
%1 = getelementptr %struct, ptr %a, i64 0, i32 1
%2 = load i64, ptr %1, align 4
ret i64 0
}

define i64 @foo(ptr %arg) {
; CHECK-LABEL: define i64 @foo(
; CHECK-SAME: ptr [[ARG:%.*]]) {
; CHECK-NEXT: [[ARG1:%.*]] = alloca [[STRUCT:%.*]], align 8
; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 16, ptr [[ARG1]])
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 1 [[ARG1]], ptr align 1 [[ARG]], i64 16, i1 false)
; CHECK-NEXT: [[TMP1:%.*]] = getelementptr [[STRUCT]], ptr [[ARG1]], i64 0, i32 1
; CHECK-NEXT: [[TMP2:%.*]] = load i64, ptr [[TMP1]], align 4
; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 16, ptr [[ARG1]])
; CHECK-NEXT: ret i64 0
;
%1 = call i64 @bar(ptr byval(%struct) align 8 %arg)
ret i64 0
}
Loading