[Driver][SPIR-V] Use consistent tools to convert between text and binary form #120266
Conversation
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
…ary form Signed-off-by: Sarnie, Nick <[email protected]>
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-spir-v @llvm/pr-subscribers-clang Author: Nick Sarnie (sarnex) ChangesCurrently we produce SPIR-V text with Once we are using the SPIR-V backend we should be able to get rid of this. Full diff: https://github.com/llvm/llvm-project/pull/120266.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/SPIRV.cpp b/clang/lib/Driver/ToolChains/SPIRV.cpp
index 659da5c7f25aa9..c84b23611bcfa6 100644
--- a/clang/lib/Driver/ToolChains/SPIRV.cpp
+++ b/clang/lib/Driver/ToolChains/SPIRV.cpp
@@ -28,8 +28,11 @@ void SPIRV::constructTranslateCommand(Compilation &C, const Tool &T,
if (Input.getType() == types::TY_PP_Asm)
CmdArgs.push_back("-to-binary");
+
+ // The text output from spirv-dis is not in the format expected
+ // by llvm-spirv, so use the text output from llvm-spirv.
if (Output.getType() == types::TY_PP_Asm)
- CmdArgs.push_back("--spirv-tools-dis");
+ CmdArgs.push_back("-to-text");
CmdArgs.append({"-o", Output.getFilename()});
diff --git a/clang/test/Driver/spirv-toolchain.cl b/clang/test/Driver/spirv-toolchain.cl
index eff02f809ce83c..bbb21d91484ffb 100644
--- a/clang/test/Driver/spirv-toolchain.cl
+++ b/clang/test/Driver/spirv-toolchain.cl
@@ -28,7 +28,7 @@
// SPT64: "-cc1" "-triple" "spirv64"
// SPT64-SAME: "-o" [[BC:".*bc"]]
-// SPT64: {{llvm-spirv.*"}} [[BC]] "--spirv-tools-dis" "-o" {{".*s"}}
+// SPT64: {{llvm-spirv.*"}} [[BC]] "-to-text" "-o" {{".*s"}}
// RUN: %clang -### --target=spirv32 -x cl -S %s 2>&1 | FileCheck --check-prefix=SPT32 %s
// RUN: %clang -### --target=spirv32 -x ir -S %s 2>&1 | FileCheck --check-prefix=SPT32 %s
@@ -37,7 +37,7 @@
// SPT32: "-cc1" "-triple" "spirv32"
// SPT32-SAME: "-o" [[BC:".*bc"]]
-// SPT32: {{llvm-spirv.*"}} [[BC]] "--spirv-tools-dis" "-o" {{".*s"}}
+// SPT32: {{llvm-spirv.*"}} [[BC]] "-to-text" "-o" {{".*s"}}
//-----------------------------------------------------------------------------
// Check assembly input -> object output
@@ -55,7 +55,7 @@
// TMP: "-cc1" "-triple" "spirv64"
// TMP-SAME: "-o" [[BC:".*bc"]]
// TMP-SAME: [[I]]
-// TMP: {{llvm-spirv.*"}} [[BC]] "--spirv-tools-dis" "-o" [[S:".*s"]]
+// TMP: {{llvm-spirv.*"}} [[BC]] "-to-text" "-o" [[S:".*s"]]
// TMP: {{llvm-spirv.*"}} [[S]] "-to-binary" "-o" {{".*o"}}
//-----------------------------------------------------------------------------
|
|
Ping on this one @llvm/pr-subscribers-backend-spir-v @llvm/pr-subscribers-clang-driver Thx! |
|
@bader Do you mind reviewing this as a member of the SPIR-V team (you already reviewed this internally) or recommending someone else to review? I'm having trouble getting reviews :) |
svenvh
requested changes
Jan 8, 2025
There was a problem hiding this comment.
Thanks for drawing this to my attention!
The output produced by llvm-spirv's -to-text is an internal textual format, intended for testing llvm-spirv only (@MrSidims please correct me if I'm wrong). I don't think we should be exposing that format through the clang driver.
What is the use case that you're trying to solve? Would it be possible to add a spirv-as invocation before passing textual SPIR-V back to llvm-spirv? Alternatively, perhaps we should teach llvm-spirv to be able to consume the conventional SPIRV-Tools textual format. That would be a natural counterpart to llvm-spirv's --spirv-tools-dis option.
Once we are using the SPIR-V backend we should be able to get rid of this.
With the current patch, I expect you'd still have the problem that llvm-spirv cannot consume the output produced by the SPIR-V backend?
|
I agree with Sven. We mustn't reuse llvm-spirv's tests format in other repositories. |
|
Thanks for the review, let me use |
|
I just reworked this to use Please take a look :) |
svenvh
approved these changes
Jan 9, 2025
MrSidims
approved these changes
Jan 9, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/15260 Here is the relevant piece of the build log for the reference |
|
This also failed one of our bots: https://lab.llvm.org/buildbot/#/builders/140/builds/14350 |
|
Yep already working on it, integration issue with another change from me, my bad |
Thank you for working on a fix. :) |
|
Fix #122310 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/15834 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/16580 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/19236 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently we produce SPIR-V text with
spirv-disbut assemble it withllvm-spirv. The SPIR-V text format is different between the tools so the assemble fails. Usespirv-asfor assembly as it uses the same format.