[SYCL] Enable nonsemantic.shader.debuginfo.200 by default #16120
Conversation
It's left disenabled only for FPGA target. Signed-off-by: Sidorov, Dmitry <[email protected]>
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| // RUN: %clang %t.devicelib.cpp -fsycl -fsycl-targets=spir64_gen-unknown-unknown -c --offload-new-driver -o %t_2.devicelib.o | ||
| // RUN: %clang %t.devicelib.cpp -fsycl -fsycl-targets=spir64_x86_64-unknown-unknown -c --offload-new-driver -o %t_3.devicelib.o | ||
|
|
||
| // Test for default llvm-spirv options |
There was a problem hiding this comment.
I haven't found the proper tests for this, so created a new one. The idea of it that when we decide to support new SPIR-V versions etc - we could modify this tests. Similar test is sycl-spirv-ext.c - but it was created for a bit different purpose and I'd like not to modify it with this patch.
There was a problem hiding this comment.
This test looks to cover the new offloading model with the changes for clang-linker-wrapper. Should an equivalent test be created for the old offloading model behaviors to check the driver defaults?
There was a problem hiding this comment.
added a new test, thanks!
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Signed-off-by: Sidorov, Dmitry <[email protected]>
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| // operations which don't have mapping to OpenCL.DebugInfo.100 spec. | ||
| TranslatorArgs.push_back("-spirv-allow-extra-diexpressions"); | ||
| } | ||
| TranslatorArgs.push_back("-spirv-debug-info-version=nonsemantic-shader-200"); |
There was a problem hiding this comment.
It looks like in the driver we don't want to do this for FPGA mode, and I don't see that implemented here, and I see at least one check for FPGA in this file:
Triple.getSubArch() != llvm::Triple::SPIRSubArch_fpga
Should we add that here?
There was a problem hiding this comment.
I can add it. Also the current version of getTripleBasedSPIRVTransOpts in ClangLinkerWrapper.cpp doesn't consider FPGA, while the logic must be exactly like in https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/Clang.cpp#L10728 . This should go into a separate patch.
There was a problem hiding this comment.
yeah thats fine for now, i guess lets just make an internal tracker to decide what we're doing with fpga
There was a problem hiding this comment.
If/When we turn on new offloading model by default in our SYCL compilation flow, we will not be using it for FPGA modes. So, it is safe to not add FPGA related logic in ClangLinkerWrapper.
Thanks
There was a problem hiding this comment.
imo we should remove that one place that checks for fpga, but definitely not related to this pr
There was a problem hiding this comment.
Thanks @sarnex. There are some unnecessary FPGA related logic which has snuck into the wrapper. That needs to be cleaned up. In my todo list. :-)
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
It's left disabled only for FPGA target.