-
Notifications
You must be signed in to change notification settings - Fork 221
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
add an entry point wrapper around functions (llvm pass) #1149
add an entry point wrapper around functions (llvm pass) #1149
Conversation
This the alternate version of #1147 using an LLVM pass. |
557c66a
to
e2f44b8
Compare
@MrSidims just wanted to make sure this version is on your radar. |
@airlied yes, I'll take a look later this day, thanks! |
lib/SPIRV/SPIRVRegularizeLLVM.cpp
Outdated
for (auto &F: Work) { | ||
|
||
// for declarations just make them into SPIR functions. | ||
if (F->empty()) { |
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.
if (F->empty()) { | |
if (F->isDeclaration()) { |
lib/SPIRV/SPIRVRegularizeLLVM.cpp
Outdated
|
||
// for declarations just make them into SPIR functions. | ||
if (F->empty()) { | ||
F->setCallingConv(CallingConv::SPIR_FUNC); | ||
continue; | ||
} |
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.
// for declarations just make them into SPIR functions. | |
if (F->empty()) { | |
F->setCallingConv(CallingConv::SPIR_FUNC); | |
continue; | |
} | |
F->setCallingConv(CallingConv::SPIR_FUNC); | |
// for declarations just make them into SPIR functions. | |
if (F->empty()) | |
continue; |
lib/SPIRV/SPIRVRegularizeLLVM.cpp
Outdated
/* have to find the spir-v metadata for execution mode and transfer it to the wrapper */ | ||
if (auto NMD = SPIRVMDWalker(*M).getNamedMD(kSPIRVMD::ExecutionMode)) { | ||
while (!NMD.atEnd()) { | ||
Function *MDF = nullptr; |
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.
Please increase indentation.
@airlied , there was not that much of a progress for about a month. Are you going to finish this? In case you need some help, I'm ready to help. |
e2f44b8
to
1ed4a5d
Compare
@Fznamznon thanks for the offer, I was waiting to see everyone was happy with the approach taken. I think I've resolved all the open review comments, however I think the big work is going to be fixing up the tests. Happy to accept any help in getting it over the line. |
b0ab52b
to
b0401ca
Compare
So this does leave the question should this do something different on the reader side? |
b0401ca
to
4c36f1d
Compare
I've added a reader side patch, and I've fixed up a bunch of tests that were expecting EntryPoint functions to have contents and matched on Name instead. There are still some tests failing, I'll have to dig some more into them. |
a14f3eb
to
65976d5
Compare
Okay making this non-draft, I've fixed up all the tests to deal with the new entrypoint vs function stuff. Those patches should be reviewed by someone who understands what those tests are meant to do, in case I did something wrong. |
else if (F->getLinkage() != GlobalValue::InternalLinkage) | ||
if (F->hasName()) { | ||
if (isKernel(F)) { | ||
/* strip the prefix as the runtime will be looking for this name */ |
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.
So, we get two functions with the same name in resulting SPIR-V, right? Maybe it will be better if they will have different names and the original kernel that contains body will have the prefix? I wonder if it may confuse some third party SPIR-V consumers.
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.
I can't figure out a way that is compatible with consumers of this library and has separate name. You can't change the non-kernel function because it breaks external linkage, and you can't change the kernel entrypoint name because it breaks runtime consumers of the translator which expect the entrypoints to have the name.
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.
You can't change the non-kernel function because it breaks external linkage
I'm not following why this is the case. If you have module A declaring a kernel, and module B calling it, if you do the same name change to both, wouldn't that then link correctly?
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.
what happens if you generate SPIR-V from something else to link in?
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.
Is that a valid way to construct an OpenCL program at the API? I know we do that with the libclc helpers, but since none of those are kernels, they wouldn't be affected, would they? Unless (I didn't really read the code) this was applying a name change to all non-kernel functions, rather than just kernel function wrappers?
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.
See KhronosGroup/SPIRV-Registry#74: This is required by OpenCL, but disallowed by SPIR-V, which is the whole point of this change.
The only question in my mind at this point is, can you take one clProgram
which is source (created from CreateProgramWithSource
), and expect to link it with another created with CreateProgramWithIL
. I'd argue no, but I don't see anything explicitly saying yes or no in the CL spec. That probably needs a separate issue on https://github.com/KhronosGroup/OpenCL-Registry.
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.
After that I wonder if it is meant to call a kernel from a separate translation unit at all.
Good question. I'm guessing this is a corner case nobody has encountered / considered before. I can see how this would be complicated given the SPIR-V constraints. Should this scenario be dis-allowed? Would that enable this PR to move forward?
The only question in my mind at this point is, can you take one clProgram which is source (created from CreateProgramWithSource), and expect to link it with another created with CreateProgramWithIL. I'd argue no, but I don't see anything explicitly saying yes or no in the CL spec.
This is interesting too, because I'd argue that this should be supported. 😄
Rationale is: the program object created from source or IL must be compiled (via clCompileProgram
) before it can be linked, which transitions the state of the program object from BINARY_TYPE_NONE
to BINARY_TYPE_COMPILED_OBJECT
. After the program object has been compiled I can't think of any reason to differentiate between program objects that originated from source and program objects that originate from IL, so the linking should "just work". Is there a detail I'm missing here?
I agree both of these topics should have separate issues for more discussion but they should be filed in in OpenCL-Docs, not OpenCL-Registry.
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.
After the program object has been compiled I can't think of any reason to differentiate between program objects that originated from source and program objects that originate from IL, so the linking should "just work". Is there a detail I'm missing here?
C++ compilers have similar problems here, where a "compiled object" from one version of a compiler doesn't necessarily have the same ABI as a "compiled object" from another version of a compiler, and therefore the two objects can't necessarily be linked. This is similar, if we treat an IL program as an object from one version of a compiler, which might not be the same as the platform's source compiler.
Should this scenario be dis-allowed? Would that enable this PR to move forward?
I don't think this is strictly necessary, what airlied has here now (two functions with the same name, one exported, one as a kernel) is probably fine, and might even be desirable since it round-trips better. While writing this comment, I've come around to this POV, but I still think it's good to probe on what the expected behavior here is - and if it is to be explicitly required, it absolutely should be tested by the CTS.
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.
I'm pretty sure it's allowed to have IL and non-IL mixed in one path. So I think this should remain as-is.
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.
Just FYI, I've created two OpenCL-Docs issues based on the above discussion:
// RUN: llvm-dis < %t.opt.bc | FileCheck %s --check-prefixes=CHECK,CHECK-W | ||
// CHECK-LABEL: spir_kernel void @foo | ||
// RUN: llvm-dis < %t.opt.bc | FileCheck %s --check-prefixes=CHECK-W | ||
// CHECK-W-LABEL: spir_func void @foo |
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.
Sorry, but I don't see why the calling convention have changed with mem2reg enabled. Could you please elaborate?
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 mem2reg test may not be testing what we expect, but the first pass just looks the raw clang outputted LLVM IR before it's been processed by regularize AFAICS, or llvm-spirv -s input.bc doesn't do what the test author things.
The fix is the first IR we test is from clang and the second is post regularizing, which suggests to me the test might actually be wrong in the first case.
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.
Yes, the test seems weird. I really didn't notice that the first check really checks LLVM IR from clang. Now I get why the change has happened, since in your changes spir_func calling convention is applied to the original kernel. Thanks.
19eec1b
to
86db497
Compare
@MrSidims , could you please help with review while I'm on vacation? |
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.
Functionally the current approach is LGTM. Please add a stand-alone test for it.
bcb2a55
to
6b52cb3
Compare
@MrSidims okay got it fixed, was one line conflict with the entrypoint spirv-1.6 changes. hopefully all the tests pass now. |
Thanks! I will merge it by 8 p.m. GMT (unless someone will have strong lastminute objections). |
std::vector<Function *> Work; | ||
|
||
// Get a list of all functions that have SPIR kernel calling conv | ||
for (auto &F : *M) { | ||
if (F.getCallingConv() == CallingConv::SPIR_KERNEL) | ||
Work.push_back(&F); | ||
} | ||
for (auto &F : Work) { |
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.
Sorry, I've just thought about this little improvement. It's just a nit:
std::vector<Function *> Work; | |
// Get a list of all functions that have SPIR kernel calling conv | |
for (auto &F : *M) { | |
if (F.getCallingConv() == CallingConv::SPIR_KERNEL) | |
Work.push_back(&F); | |
} | |
for (auto &F : Work) { | |
for (auto &F : *M) { | |
// Skip functions that have not a SPIR kernel calling conv | |
if (F.getCallingConv() != CallingConv::SPIR_KERNEL) | |
continue; |
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.
I'll apply this comment after the original request is merged.
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.
Not sure if that's a valid suggestion, as the loop may modify the Module's function list (using getOrCreateFunction
) while its being iterated over.
Now that this change has been merged, can KhronosGroup/SPIRV-Registry#74 be closed? |
…#1149) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation.
…s) (#1149) (#1581) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation. Co-authored-by: Dave Airlie <airlied@redhat.com>
…1149) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation.
…ss) (#1149) (#1584) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation. Co-authored-by: Dave Airlie <airlied@redhat.com>
…ss) (#1149) (#1585) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation. Co-authored-by: Dave Airlie <airlied@redhat.com>
…ss) (#1149) (#1583) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation. Co-authored-by: Dave Airlie <airlied@redhat.com>
…(llvm pass) (#.. '"master"' -> '"xmain-web"' (20 commits) CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVRegularizeLLVM.cpp commit 85815e7 Author: Dave Airlie <airlied@redhat.com> Date: Tue Jan 25 10:17:44 2022 +1000 Add an entry point wrapper around functions (llvm pass) (KhronosGroup#1149) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation.
… functions (llvm pass) (#.. '"master"' -> '"xmain-web"' (20 commits) (KhronosGroup#103) * Resolve of merge (conflict) 85815e7 Add an entry point wrapper around functions (llvm pass) (#.. '"master"' -> '"xmain-web"' (20 commits) CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVRegularizeLLVM.cpp commit 85815e7 Author: Dave Airlie <airlied@redhat.com> Date: Tue Jan 25 10:17:44 2022 +1000 Add an entry point wrapper around functions (llvm pass) (KhronosGroup#1149) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation. * Resolve conflicts Co-authored-by: iclsrc <ia.compiler.tools.git@intel.com> Co-authored-by: Sergey Semenov <sergey.semenov@intel.com>
…er around functions (llvm pass) (#.. '"master"' -> '"xmain-web"' (20 commits) (KhronosGroup#103) * Resolve of merge (conflict) 85815e7 Add an entry point wrapper around functions (llvm pass) (#.. '"master"' -> '"xmain-web"' (20 commits) CONFLICT (content): Merge conflict in lib/SPIRV/SPIRVRegularizeLLVM.cpp commit 85815e7 Author: Dave Airlie <airlied@redhat.com> Date: Tue Jan 25 10:17:44 2022 +1000 Add an entry point wrapper around functions (llvm pass) (KhronosGroup#1149) SPIR-V spec states: "It is invalid for any function to be targeted by both an OpEntryPoint instruction and an OpFunctionCall instruction." In order to satisfy SPIR-V that entrypoints and functions must be different, this introduces an entrypoint wrapper around functions at the LLVM IR level, then fixes up a few things like naming at the SPIRV translation. * Resolve conflicts Co-authored-by: iclsrc <ia.compiler.tools.git@intel.com> Co-authored-by: Sergey Semenov <sergey.semenov@intel.com>
This reverts: commit 85815e7 Author: Dave Airlie <airlied@redhat.com> Date: Tue Jan 25 10:17:44 2022 +1000 Add an entry point wrapper around functions (llvm pass) (KhronosGroup#1149) Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
This reverts: commit 85815e7 Author: Dave Airlie <airlied@redhat.com> Date: Tue Jan 25 10:17:44 2022 +1000 Add an entry point wrapper around functions (llvm pass) (KhronosGroup#1149) Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com> Co-authored-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
This patch is a result of a reflection about previously merged PR KhronosGroup#1149 "add an entry point wrapper around functions (llvm pass)" and is enspired by various reported translator, clang (OpenCL) and Intel GPU drivers issues (see KhronosGroup#2029 for reference). While SPIR-V spec states: === *OpName* --//--. This has nosemantic impact and can safely be removed from a module. === yet having EntryPoint function and a function that shares the name via OpName might be confusing by both (old) drivers and programmers, who read the SPIR-V file. This patch prevents generation of the wrapper function when it's not necessary to generate it aka if a kernel function is not called by other kernel. We can do better in other cases as well, for example I have experiments of renaming a wrapped function adding a previx, so it could be distinguished from the actual kernel/entry point, but for now it doesn't pass validation for E2E OpenCL tests. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
* Don't wrap kernels that are not being called in the module This patch is a result of a reflection about previously merged PR #1149 "add an entry point wrapper around functions (llvm pass)" and is enspired by various reported translator, clang (OpenCL) and Intel GPU drivers issues (see #2029 for reference). While SPIR-V spec states: === *OpName* --//--. This has nosemantic impact and can safely be removed from a module. === yet having EntryPoint function and a function that shares the name via OpName might be confusing by both (old) drivers and programmers, who read the SPIR-V file. This patch prevents generation of the wrapper function when it's not necessary to generate it aka if a kernel function is not called by other kernel. We can do better in other cases as well, for example I have experiments of renaming a wrapped function adding a previx, so it could be distinguished from the actual kernel/entry point, but for now it doesn't pass validation for E2E OpenCL tests. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com> * prevent a copy Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com> This patch is a result of a reflection about previously merged PR #1149 "add an entry point wrapper around functions (llvm pass)" and is enspired by various reported translator, clang (OpenCL) and Intel GPU drivers issues (see #2029 for reference). While SPIR-V spec states: OpName --//--. This has nosemantic impact and can safely be removed from a module. yet having EntryPoint function and a function that shares the name via OpName might be confusing by both not-up-to-date drivers and programmers, who read the SPIR-V file. This patch prevents generation of the wrapper function when it's not necessary to generate it aka if a kernel function is not called by other kernel. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
* Don't wrap kernels that are not being called in the module This patch is a result of a reflection about previously merged PR KhronosGroup/SPIRV-LLVM-Translator#1149 "add an entry point wrapper around functions (llvm pass)" and is enspired by various reported translator, clang (OpenCL) and Intel GPU drivers issues (see KhronosGroup/SPIRV-LLVM-Translator#2029 for reference). While SPIR-V spec states: === *OpName* --//--. This has nosemantic impact and can safely be removed from a module. === yet having EntryPoint function and a function that shares the name via OpName might be confusing by both (old) drivers and programmers, who read the SPIR-V file. This patch prevents generation of the wrapper function when it's not necessary to generate it aka if a kernel function is not called by other kernel. We can do better in other cases as well, for example I have experiments of renaming a wrapped function adding a previx, so it could be distinguished from the actual kernel/entry point, but for now it doesn't pass validation for E2E OpenCL tests. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com> * prevent a copy Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com> This patch is a result of a reflection about previously merged PR #1149 "add an entry point wrapper around functions (llvm pass)" and is enspired by various reported translator, clang (OpenCL) and Intel GPU drivers issues (see While SPIR-V spec states: OpName --//--. This has nosemantic impact and can safely be removed from a module. yet having EntryPoint function and a function that shares the name via OpName might be confusing by both not-up-to-date drivers and programmers, who read the SPIR-V file. This patch prevents generation of the wrapper function when it's not necessary to generate it aka if a kernel function is not called by other kernel. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com> Original commit: KhronosGroup/SPIRV-LLVM-Translator@46285e4
…oup#2119) * Don't wrap kernels that are not being called in the module This patch is a result of a reflection about previously merged PR KhronosGroup#1149 "add an entry point wrapper around functions (llvm pass)" and is enspired by various reported translator, clang (OpenCL) and Intel GPU drivers issues (see KhronosGroup#2029 for reference). While SPIR-V spec states: === *OpName* --//--. This has nosemantic impact and can safely be removed from a module. === yet having EntryPoint function and a function that shares the name via OpName might be confusing by both (old) drivers and programmers, who read the SPIR-V file. This patch prevents generation of the wrapper function when it's not necessary to generate it aka if a kernel function is not called by other kernel. We can do better in other cases as well, for example I have experiments of renaming a wrapped function adding a previx, so it could be distinguished from the actual kernel/entry point, but for now it doesn't pass validation for E2E OpenCL tests. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com> * prevent a copy Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com> This patch is a result of a reflection about previously merged PR KhronosGroup#1149 "add an entry point wrapper around functions (llvm pass)" and is enspired by various reported translator, clang (OpenCL) and Intel GPU drivers issues (see KhronosGroup#2029 for reference). While SPIR-V spec states: OpName --//--. This has nosemantic impact and can safely be removed from a module. yet having EntryPoint function and a function that shares the name via OpName might be confusing by both not-up-to-date drivers and programmers, who read the SPIR-V file. This patch prevents generation of the wrapper function when it's not necessary to generate it aka if a kernel function is not called by other kernel. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
SPIR-V spec states:
"It is invalid for any function to be targeted by both an OpEntryPoint instruction
and an OpFunctionCall instruction."
In order to satisfy SPIR-V that entrypoints and functions
must be different, this introduces an entrypoint wrapper around
functions at the LLVM IR level, then fixes up a few things like
naming at the SPIRV translation.