-
Notifications
You must be signed in to change notification settings - Fork 802
[Driver] Set default value for opaque pointers in offload bundler #9933
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
Changes from 10 commits
c7db9d6
c96918c
ec1f09e
69a3d82
93fa364
6e4eb68
af2022e
ce453d3
0744ea6
52ccb85
6b85d26
1c7b777
3fa4db3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /// | ||
| /// Checks that opaque-pointers are being set for the llvm context accordingly to the bundle targets, | ||
| /// such that for multiple targets, if the first bc input module uses typed pointers, the bitcode | ||
| /// reader will not fail because opaque pointers cannot be converted to typed pointers. | ||
| /// | ||
|
|
||
| #include <sycl/sycl.hpp> | ||
|
||
|
|
||
| int main() { | ||
| sycl::queue q; | ||
| auto ptr = sycl::malloc_shared<int>(1, q); | ||
| q.single_task([=]() { | ||
| ptr[0] = 1; | ||
| }); | ||
| return 0; | ||
| } | ||
|
|
||
| // UNSUPPORTED: system-windows | ||
|
|
||
| // | ||
| // Generate inputs for clang-offload-bundler. | ||
| // | ||
| // RUN: %clangxx -fsycl -fsycl-targets=spir64 -fsycl-device-only %s -c -emit-llvm -o %s.spir64.bc | ||
| // RUN: %clangxx -fsycl -fsycl-targets=nvptx-nvidia-cuda -fsycl-device-only %s -c -emit-llvm -o %s.nvptx.bc | ||
| // RUN: %clangxx -fsycl -target x86_64-unknown-linux-gnu %s -c -o %s.host.o | ||
| // -fsycl -fno-sycl-instrument-device-code -fno-sycl-device-lib=all | ||
|
|
||
| // | ||
| // Check clang-offload-bundler obj for opaque pointers support error when spir64 with | ||
| // is the first input, followed by other opaque pointers modules, i.e., here nvptx64. | ||
| // | ||
| // RUN: clang-offload-bundler -type=o -targets=sycl-spir64-unknown-unknown,sycl-nvptx64-nvidia-cuda-sm_50,host-x86_64-unknown-linux-gpu -output=%s.o -input=%s.spir64.bc -input=%s.nvptx.bc -input=%s.host.o 2>&1 \ | ||
| // RUN: | FileCheck %s -check-prefix=CHECK-STD --allow-empty | ||
| // CHECK-STD-NOT: error: Opaque pointers are only supported in -opaque-pointers mode | ||
|
|
||
| // | ||
| // Check the nvptx module actually contains opaque pointers and no typed pointers. | ||
| // | ||
| // RUN: llvm-dis %s.nvptx.bc -o %s.nvptx.ll | ||
| // RUN: FileCheck %s --check-prefix SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_NVPTX --input-file=%s.nvptx.ll | ||
| // SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_NVPTX: target triple = "nvptx-nvidia-cuda" | ||
| // SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_NVPTX: ptr addrspace(1) | ||
| // SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_NVPTX-NOT: i32 addrspace(1)* | ||
|
|
||
| // | ||
| // Check the spir64 module also contains opaque pointers. | ||
| // | ||
| // SPIR targets now have opaque pointer support, hence we just double check it. | ||
| // They didn't before, so the check was the other way around verifying typed ptrs. | ||
| // | ||
| // RUN: llvm-dis %s.spir64.bc -o %s.spir64.ll | ||
| // RUN: FileCheck %s --check-prefix SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_SPIR --input-file=%s.spir64.ll | ||
| // SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_SPIR: target triple = "spir64-unknown-unknown" | ||
| // SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_SPIR-NOT: i32 addrspace(1)* | ||
| // SYCL_OFFLOAD_BUNDLER_MULTI_TARGET_OPAQUE_POINTERS_SPIR: ptr addrspace(1) | ||
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 poking around in more detail (I don't regularly develop this tool), I've come to the conclusion that this code is an elaborate way to set
Context.setOpaquePointers(true). The host is included inTargetNames, and can't be SPIRV anyways, so there's always a non-SPIR-V module to cause it to be set to true. This of course obscures the failure modes of what would happen ifUseOpaquePointerswere false, since there's no realistic mode where it could be false.Given that SPIRV_ENABLE_OPAQUE_POINTERS is enabled now and we're starting to remove support for typed pointers, I'm not sure there's any need for this PR anymore.
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.
Do you mean
LLVMContext Context;uses opaque pointers by default and don't need to switch to non-opaque mode anymore?@GeorgeWeb, are you okay to drop this change?
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.
#10888 means INTEL_SYCL_OPAQUEPOINTER_READY is now true and therefore everything is always in opaque pointer mode.
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.
Always?
Why do we add ifdef like this:
?
Shouldn't we just remove this code?
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.
In principle, it's a cmake option you can turn off. However, I suspect it doesn't actually work anymore. And I also suspect @jsji is likely to start ripping the code out in the near future, after the code bakes for a little bit more (the PR landed only yesterday).
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 plan is to remove the code in #ifndef INTEL_SYCL_OPAQUEPOINTER_READY once our downstream code works well.
ETA 1-2 weeks.
Keep it by guarding it within macros just in case we need to compare to INTEL_SYCL_OPAQUEPOINTER_READY=0 in some case for now.
Uh oh!
There was an error while loading. Please reload this page.
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. Also I believe
setOpaquePointersis now an assert, so don't call it anymore.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 see! So, assuming
INTEL_SYCL_OPAQUEPOINTER_READYgoes away soon-ish then it's all gonna look cleaner. My patch was I guess really a temporary mitigation for an awkward issue and from the insights of this discussion - thanks @jsji and @jcranmer-intel I am happy to close this PR as it seems unneeded forintel:sycl. 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.
@bader If you are convinced too, I am happy to close this given the future direction.