[HLSL][SPIRV] Add CLI option -fspv-extension
#137985
Merged
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
This commit implements DXC's `-fspv-extension` options. It is implemented by replaced it with the equivalent `-spirv-ext` option. Note that if the option is not used, that is the same as enabling all extension, so `-spirv-ext=all` is used. Fixes llvm#137647
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Steven Perron (s-perron) ChangesThis commit implements DXC's Fixes #137647 Full diff: https://github.com/llvm/llvm-project/pull/137985.diff 3 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index a9f4083920663..a9e9ca5cc2c95 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -9213,6 +9213,11 @@ def metal : DXCFlag<"metal">, HelpText<"Generate Metal library">;
def fspv_target_env_EQ : Joined<["-"], "fspv-target-env=">, Group<dxc_Group>,
HelpText<"Specify the target environment">,
Values<"vulkan1.2, vulkan1.3">;
+def fspv_extension_EQ
+ : Joined<["-"], "fspv-extension=">,
+ Group<dxc_Group>,
+ HelpText<"Specify the available SPIR-V extensions. If this option is not "
+ "specificed, then all extensions are available.">;
def no_wasm_opt : Flag<["--"], "no-wasm-opt">,
Group<m_Group>,
HelpText<"Disable the wasm-opt optimizer">,
diff --git a/clang/lib/Driver/ToolChains/HLSL.cpp b/clang/lib/Driver/ToolChains/HLSL.cpp
index 59e9050af8a76..b71e27c20043a 100644
--- a/clang/lib/Driver/ToolChains/HLSL.cpp
+++ b/clang/lib/Driver/ToolChains/HLSL.cpp
@@ -13,6 +13,7 @@
#include "clang/Driver/Job.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/TargetParser/Triple.h"
+#include <regex>
using namespace clang::driver;
using namespace clang::driver::tools;
@@ -173,6 +174,39 @@ bool isLegalValidatorVersion(StringRef ValVersionStr, const Driver &D) {
return true;
}
+std::string getSpirvExtArg(ArrayRef<std::string> SpvExtensionArgs) {
+ if (SpvExtensionArgs.empty()) {
+ return "-spirv-ext=all";
+ }
+
+ std::string LlvmOption =
+ (Twine("-spirv-ext=+") + SpvExtensionArgs.front()).str();
+ SpvExtensionArgs = SpvExtensionArgs.slice(1);
+ for (auto Extension : SpvExtensionArgs) {
+ LlvmOption = (Twine(LlvmOption) + ",+" + Extension).str();
+ }
+ return LlvmOption;
+}
+
+bool isValidSPIRVExtensionName(const std::string &str) {
+ std::regex pattern("SPV_[a-zA-Z0-9_]+");
+ return std::regex_match(str, pattern);
+}
+
+// SPIRV extension names are of the form `SPV_[a-zA-Z0-9_]+`. We want to
+// disallow obviously invalid names to avoid issues when parsing `spirv-ext`.
+bool checkExtensionArgsAreValid(ArrayRef<std::string> SpvExtensionArgs,
+ const Driver &Driver) {
+ bool AllValid = true;
+ for (auto Extension : SpvExtensionArgs) {
+ if (!isValidSPIRVExtensionName(Extension)) {
+ Driver.Diag(diag::err_drv_invalid_value)
+ << "-fspv_extension" << Extension;
+ AllValid = false;
+ }
+ }
+ return AllValid;
+}
} // namespace
void tools::hlsl::Validator::ConstructJob(Compilation &C, const JobAction &JA,
@@ -301,6 +335,17 @@ HLSLToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch,
DAL->append(A);
}
+ if (getArch() == llvm::Triple::spirv) {
+ std::vector<std::string> SpvExtensionArgs =
+ Args.getAllArgValues(options::OPT_fspv_extension_EQ);
+ if (checkExtensionArgsAreValid(SpvExtensionArgs, getDriver())) {
+ std::string LlvmOption = getSpirvExtArg(SpvExtensionArgs);
+ DAL->AddSeparateArg(nullptr, Opts.getOption(options::OPT_mllvm),
+ LlvmOption);
+ }
+ Args.claimAllArgs(options::OPT_fspv_extension_EQ);
+ }
+
if (!DAL->hasArg(options::OPT_O_Group)) {
DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_O), "3");
}
diff --git a/clang/test/Driver/dxc_fspv_extension.hlsl b/clang/test/Driver/dxc_fspv_extension.hlsl
new file mode 100644
index 0000000000000..0a9d321b8d95e
--- /dev/null
+++ b/clang/test/Driver/dxc_fspv_extension.hlsl
@@ -0,0 +1,25 @@
+// If there is no `-fspv-extension`, enable all extensions.
+// RUN: %clang_dxc -spirv -Tlib_6_7 -### %s 2>&1 | FileCheck %s -check-prefix=ALL
+// ALL: "-spirv-ext=all"
+
+// Convert the `-fspv-extension` into `spirv-ext`.
+// RUN: %clang_dxc -spirv -Tlib_6_7 -### %s -fspv-extension=SPV_TEST1 2>&1 | FileCheck %s -check-prefix=TEST1
+// TEST1: "-spirv-ext=+SPV_TEST1"
+
+// Merge multiple extensions into a single `spirv-ext` option.
+// RUN: %clang_dxc -spirv -Tlib_6_7 -### %s -fspv-extension=SPV_TEST1 -fspv-extension=SPV_TEST2 2>&1 | FileCheck %s -check-prefix=TEST2
+// TEST2: "-spirv-ext=+SPV_TEST1,+SPV_TEST2"
+
+// Check for the error message if the extension name is not properly formed.
+// RUN: not %clang_dxc -spirv -Tlib_6_7 -### %s -fspv-extension=TEST1 -fspv-extension=SPV_GOOD -fspv-extension=TEST2 2>&1 | FileCheck %s -check-prefix=FAIL
+// FAIL: invalid value 'TEST1' in '-fspv_extension'
+// FAIL: invalid value 'TEST2' in '-fspv_extension'
+
+// If targeting DXIL, the `-spirv-ext` should not be passed to the backend.
+// RUN: %clang_dxc -Tlib_6_7 -### %s 2>&1 | FileCheck %s -check-prefix=DXIL
+// DXIL-NOT: spirv-ext
+
+// If targeting DXIL, the `-fspv-extension` option is meaningless, and there should be a warning.
+// RUN: %clang_dxc -Tlib_6_7 -### %s -fspv-extension=SPV_TEST 2>&1 | FileCheck %s -check-prefix=WARN
+// WARN: warning: argument unused during compilation: '-fspv-extension=SPV_TEST'
+// WARN-NOT: spirv-ext
|
|
@llvm/pr-subscribers-hlsl Author: Steven Perron (s-perron) ChangesThis commit implements DXC's Fixes #137647 Full diff: https://github.com/llvm/llvm-project/pull/137985.diff 3 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index a9f4083920663..a9e9ca5cc2c95 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -9213,6 +9213,11 @@ def metal : DXCFlag<"metal">, HelpText<"Generate Metal library">;
def fspv_target_env_EQ : Joined<["-"], "fspv-target-env=">, Group<dxc_Group>,
HelpText<"Specify the target environment">,
Values<"vulkan1.2, vulkan1.3">;
+def fspv_extension_EQ
+ : Joined<["-"], "fspv-extension=">,
+ Group<dxc_Group>,
+ HelpText<"Specify the available SPIR-V extensions. If this option is not "
+ "specificed, then all extensions are available.">;
def no_wasm_opt : Flag<["--"], "no-wasm-opt">,
Group<m_Group>,
HelpText<"Disable the wasm-opt optimizer">,
diff --git a/clang/lib/Driver/ToolChains/HLSL.cpp b/clang/lib/Driver/ToolChains/HLSL.cpp
index 59e9050af8a76..b71e27c20043a 100644
--- a/clang/lib/Driver/ToolChains/HLSL.cpp
+++ b/clang/lib/Driver/ToolChains/HLSL.cpp
@@ -13,6 +13,7 @@
#include "clang/Driver/Job.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/TargetParser/Triple.h"
+#include <regex>
using namespace clang::driver;
using namespace clang::driver::tools;
@@ -173,6 +174,39 @@ bool isLegalValidatorVersion(StringRef ValVersionStr, const Driver &D) {
return true;
}
+std::string getSpirvExtArg(ArrayRef<std::string> SpvExtensionArgs) {
+ if (SpvExtensionArgs.empty()) {
+ return "-spirv-ext=all";
+ }
+
+ std::string LlvmOption =
+ (Twine("-spirv-ext=+") + SpvExtensionArgs.front()).str();
+ SpvExtensionArgs = SpvExtensionArgs.slice(1);
+ for (auto Extension : SpvExtensionArgs) {
+ LlvmOption = (Twine(LlvmOption) + ",+" + Extension).str();
+ }
+ return LlvmOption;
+}
+
+bool isValidSPIRVExtensionName(const std::string &str) {
+ std::regex pattern("SPV_[a-zA-Z0-9_]+");
+ return std::regex_match(str, pattern);
+}
+
+// SPIRV extension names are of the form `SPV_[a-zA-Z0-9_]+`. We want to
+// disallow obviously invalid names to avoid issues when parsing `spirv-ext`.
+bool checkExtensionArgsAreValid(ArrayRef<std::string> SpvExtensionArgs,
+ const Driver &Driver) {
+ bool AllValid = true;
+ for (auto Extension : SpvExtensionArgs) {
+ if (!isValidSPIRVExtensionName(Extension)) {
+ Driver.Diag(diag::err_drv_invalid_value)
+ << "-fspv_extension" << Extension;
+ AllValid = false;
+ }
+ }
+ return AllValid;
+}
} // namespace
void tools::hlsl::Validator::ConstructJob(Compilation &C, const JobAction &JA,
@@ -301,6 +335,17 @@ HLSLToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch,
DAL->append(A);
}
+ if (getArch() == llvm::Triple::spirv) {
+ std::vector<std::string> SpvExtensionArgs =
+ Args.getAllArgValues(options::OPT_fspv_extension_EQ);
+ if (checkExtensionArgsAreValid(SpvExtensionArgs, getDriver())) {
+ std::string LlvmOption = getSpirvExtArg(SpvExtensionArgs);
+ DAL->AddSeparateArg(nullptr, Opts.getOption(options::OPT_mllvm),
+ LlvmOption);
+ }
+ Args.claimAllArgs(options::OPT_fspv_extension_EQ);
+ }
+
if (!DAL->hasArg(options::OPT_O_Group)) {
DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_O), "3");
}
diff --git a/clang/test/Driver/dxc_fspv_extension.hlsl b/clang/test/Driver/dxc_fspv_extension.hlsl
new file mode 100644
index 0000000000000..0a9d321b8d95e
--- /dev/null
+++ b/clang/test/Driver/dxc_fspv_extension.hlsl
@@ -0,0 +1,25 @@
+// If there is no `-fspv-extension`, enable all extensions.
+// RUN: %clang_dxc -spirv -Tlib_6_7 -### %s 2>&1 | FileCheck %s -check-prefix=ALL
+// ALL: "-spirv-ext=all"
+
+// Convert the `-fspv-extension` into `spirv-ext`.
+// RUN: %clang_dxc -spirv -Tlib_6_7 -### %s -fspv-extension=SPV_TEST1 2>&1 | FileCheck %s -check-prefix=TEST1
+// TEST1: "-spirv-ext=+SPV_TEST1"
+
+// Merge multiple extensions into a single `spirv-ext` option.
+// RUN: %clang_dxc -spirv -Tlib_6_7 -### %s -fspv-extension=SPV_TEST1 -fspv-extension=SPV_TEST2 2>&1 | FileCheck %s -check-prefix=TEST2
+// TEST2: "-spirv-ext=+SPV_TEST1,+SPV_TEST2"
+
+// Check for the error message if the extension name is not properly formed.
+// RUN: not %clang_dxc -spirv -Tlib_6_7 -### %s -fspv-extension=TEST1 -fspv-extension=SPV_GOOD -fspv-extension=TEST2 2>&1 | FileCheck %s -check-prefix=FAIL
+// FAIL: invalid value 'TEST1' in '-fspv_extension'
+// FAIL: invalid value 'TEST2' in '-fspv_extension'
+
+// If targeting DXIL, the `-spirv-ext` should not be passed to the backend.
+// RUN: %clang_dxc -Tlib_6_7 -### %s 2>&1 | FileCheck %s -check-prefix=DXIL
+// DXIL-NOT: spirv-ext
+
+// If targeting DXIL, the `-fspv-extension` option is meaningless, and there should be a warning.
+// RUN: %clang_dxc -Tlib_6_7 -### %s -fspv-extension=SPV_TEST 2>&1 | FileCheck %s -check-prefix=WARN
+// WARN: warning: argument unused during compilation: '-fspv-extension=SPV_TEST'
+// WARN-NOT: spirv-ext
|
bogner
approved these changes
Apr 30, 2025
cassiebeckley
approved these changes
May 1, 2025
Keenuts
approved these changes
May 2, 2025
Co-authored-by: Cassandra Beckley <cbeckley@google.com>
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.
This commit implements DXC's
-fspv-extensionoptions. It isimplemented by replaced it with the equivalent
-spirv-extoption. Notethat if the option is not used, that is the same as enabling all
extension, so
-spirv-ext=allis used.Fixes #137647