-
Notifications
You must be signed in to change notification settings - Fork 16.3k
[SPIRV] Extend lowering of variadic functions #178980
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.
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
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
41 changes: 41 additions & 0 deletions
41
llvm/test/CodeGen/SPIRV/function/variadics-lowering-namespace-printf.ll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| ; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown --spirv-ext=+SPV_INTEL_function_pointers < %s 2>&1 | FileCheck %s | ||
|
|
||
| ; CHECK: OpName [[MAIN:%.*]] "main" | ||
| ; CHECK: [[MAIN]] = OpFunction | ||
| ; CHECK-NOT: OPFunctionEnd | ||
| ; CHECK: OpLifetimeStart | ||
| ; CHECK-NOT: OPFunctionEnd | ||
| ; CHECK: OpLifetimeStop | ||
| ; CHECK: OpFunctionEnd | ||
|
|
||
| @.str = private unnamed_addr addrspace(1) constant [3 x i8] c"%s\00", align 1 | ||
| @.str.1 = private unnamed_addr addrspace(1) constant [4 x i8] c"hey\00", align 1 | ||
|
|
||
| declare spir_func noundef i32 @_Z7vprintfPKcz(ptr addrspace(4) noundef %0, ...) addrspace(9) | ||
|
|
||
| define spir_func noundef i32 @_ZN4ompx6printfEPKcz(ptr addrspace(4) noundef %Format, ...) addrspace(9) { | ||
| entry: | ||
| %retval = alloca i32, align 4 | ||
| %Format.addr = alloca ptr addrspace(4), align 8 | ||
| %vlist = alloca ptr addrspace(4), align 8 | ||
| %retval.ascast = addrspacecast ptr %retval to ptr addrspace(4) | ||
| %Format.addr.ascast = addrspacecast ptr %Format.addr to ptr addrspace(4) | ||
| %vlist.ascast = addrspacecast ptr %vlist to ptr addrspace(4) | ||
| store ptr addrspace(4) %Format, ptr addrspace(4) %Format.addr.ascast, align 8 | ||
| call addrspace(9) void @llvm.va_start.p4(ptr addrspace(4) %vlist.ascast) | ||
| %0 = load ptr addrspace(4), ptr addrspace(4) %Format.addr.ascast, align 8 | ||
| %1 = load ptr addrspace(4), ptr addrspace(4) %vlist.ascast, align 8 | ||
| %call = call spir_func noundef addrspace(9) i32 (ptr addrspace(4), ...) @_Z7vprintfPKcz(ptr addrspace(4) noundef %0, ptr addrspace(4) noundef %1) | ||
| ret i32 %call | ||
| } | ||
|
|
||
| declare void @llvm.va_start.p4(ptr addrspace(4)) addrspace(9) | ||
|
|
||
| define noundef i32 @main() addrspace(9) { | ||
| entry: | ||
| %retval = alloca i32, align 4 | ||
| %retval.ascast = addrspacecast ptr %retval to ptr addrspace(4) | ||
| store i32 0, ptr addrspace(4) %retval.ascast, align 4 | ||
| %call = call spir_func noundef addrspace(9) i32 (ptr addrspace(4), ...) @_ZN4ompx6printfEPKcz(ptr addrspace(4) noundef addrspacecast (ptr addrspace(1) @.str to ptr addrspace(4)), ptr addrspace(4) noundef addrspacecast (ptr addrspace(1) @.str.1 to ptr addrspace(4))) #5 | ||
| ret i32 0 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason this handling existed was to preserve the 'old' printf handling. It seems a little strange to only lower functions based off of the address space, this is supposed to be the canonical variadic ABI for the target. Ideally we wouldn't leave anything unlowered. I mostly hacked around the
printfissue because I don't know anything about its intended use.Here's a fun hack if you just want the OpenMP printf to work, just rename it with
asm("foobar").There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The address space handing was there previously and you didn't modify it in your change. The existing code has some weird logic about address spaces, only lowering 0 and the alloca AS. I have no idea why that is, but I think it's safer to add a known-good case than start processing everything for all targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhuber6 Did you have any more feedback on this PR? I think there was a bit of a misunderstanding before, the proposed changes to
ExpandVariadicsshould not be specific toprintf(in the latest iteration of the PR), this PR actually aims to remove all of that, and I think the addition of handling target address spaces fits reasonably within the existing code that already has a list of address spaces to try to lower. Thanks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhuber6 Mind taking another look at this with the above context? If you don't feel comfortable reviewing I can try to find someone else, thanks