[SPIRV] Filter disallowed extensions for env #150051
Open
+120
−2
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
Not all SPIR-V extensions are allows in every environment. When we use the `-spirv-ext=all` option, the backend currently believes that all extensions can be used. This commit filters out the extensions on the command line to remove those that are not known to be allowed for the current environment. Alternatives considered: I considered modifying the SPIRVExtensionsParser::parse to use a different list of extensions for "all" depending on the target triple. However that does not work because the target triple is not available, and cannot be made available in a reasonable way. Fixes llvm#147717
|
@llvm/pr-subscribers-backend-spir-v Author: Steven Perron (s-perron) ChangesNot all SPIR-V extensions are allows in every environment. When we use This commit filters out the extensions on the command line to remove Alternatives considered: I considered modifying the Fixes #147717 Full diff: https://github.com/llvm/llvm-project/pull/150051.diff 4 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp b/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
index 2726203d253ad..be3a6a93a5a51 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
@@ -12,7 +12,7 @@
//===----------------------------------------------------------------------===//
#include "SPIRVCommandLine.h"
-#include "llvm/ADT/StringRef.h"
+#include "llvm/TargetParser/Triple.h"
#include <algorithm>
#include <map>
@@ -20,6 +20,69 @@
using namespace llvm;
+// List of extensions allowed for Vulkan environment.
+// This should not contain an extension that is not listed in
+// https://github.com/KhronosGroup/Vulkan-Headers/blob/main/registry/vk.xml.
+static const std::set<SPIRV::Extension::Extension> ValidVulkanExtensions = {
+ SPIRV::Extension::SPV_EXT_demote_to_helper_invocation,
+ SPIRV::Extension::SPV_EXT_shader_atomic_float_add,
+ SPIRV::Extension::SPV_EXT_shader_atomic_float16_add,
+ SPIRV::Extension::SPV_EXT_shader_atomic_float_min_max,
+ SPIRV::Extension::SPV_KHR_cooperative_matrix,
+ SPIRV::Extension::SPV_KHR_expect_assume,
+ SPIRV::Extension::SPV_KHR_float_controls,
+ SPIRV::Extension::SPV_KHR_float_controls2,
+ SPIRV::Extension::SPV_KHR_integer_dot_product,
+ SPIRV::Extension::SPV_KHR_non_semantic_info,
+ SPIRV::Extension::SPV_KHR_shader_clock,
+ SPIRV::Extension::SPV_KHR_subgroup_rotate};
+
+// List of extensions allowed for OpenCL environment.
+// TODO: Remove extension not used by OpenCL. I do not know how to get that
+// information.
+static const std::set<SPIRV::Extension::Extension> ValidOpenCLExtensions = {
+ SPIRV::Extension::SPV_EXT_shader_atomic_float_add,
+ SPIRV::Extension::SPV_EXT_shader_atomic_float16_add,
+ SPIRV::Extension::SPV_EXT_shader_atomic_float_min_max,
+ SPIRV::Extension::SPV_EXT_arithmetic_fence,
+ SPIRV::Extension::SPV_EXT_demote_to_helper_invocation,
+ SPIRV::Extension::SPV_INTEL_arbitrary_precision_integers,
+ SPIRV::Extension::SPV_INTEL_cache_controls,
+ SPIRV::Extension::SPV_INTEL_float_controls2,
+ SPIRV::Extension::SPV_INTEL_global_variable_fpga_decorations,
+ SPIRV::Extension::SPV_INTEL_global_variable_host_access,
+ SPIRV::Extension::SPV_INTEL_optnone,
+ SPIRV::Extension::SPV_EXT_optnone,
+ SPIRV::Extension::SPV_INTEL_usm_storage_classes,
+ SPIRV::Extension::SPV_INTEL_split_barrier,
+ SPIRV::Extension::SPV_INTEL_subgroups,
+ SPIRV::Extension::SPV_INTEL_media_block_io,
+ SPIRV::Extension::SPV_INTEL_memory_access_aliasing,
+ SPIRV::Extension::SPV_INTEL_joint_matrix,
+ SPIRV::Extension::SPV_KHR_uniform_group_instructions,
+ SPIRV::Extension::SPV_KHR_no_integer_wrap_decoration,
+ SPIRV::Extension::SPV_KHR_float_controls,
+ SPIRV::Extension::SPV_KHR_expect_assume,
+ SPIRV::Extension::SPV_KHR_bit_instructions,
+ SPIRV::Extension::SPV_KHR_integer_dot_product,
+ SPIRV::Extension::SPV_KHR_linkonce_odr,
+ SPIRV::Extension::SPV_INTEL_inline_assembly,
+ SPIRV::Extension::SPV_INTEL_bindless_images,
+ SPIRV::Extension::SPV_INTEL_bfloat16_conversion,
+ SPIRV::Extension::SPV_KHR_subgroup_rotate,
+ SPIRV::Extension::SPV_INTEL_variable_length_array,
+ SPIRV::Extension::SPV_INTEL_function_pointers,
+ SPIRV::Extension::SPV_KHR_shader_clock,
+ SPIRV::Extension::SPV_KHR_cooperative_matrix,
+ SPIRV::Extension::SPV_KHR_non_semantic_info,
+ SPIRV::Extension::SPV_INTEL_long_composites,
+ SPIRV::Extension::SPV_INTEL_fp_max_error,
+ SPIRV::Extension::SPV_INTEL_subgroup_matrix_multiply_accumulate,
+ SPIRV::Extension::SPV_INTEL_ternary_bitwise_function,
+ SPIRV::Extension::SPV_INTEL_2d_block_io,
+ SPIRV::Extension::SPV_INTEL_int4,
+ SPIRV::Extension::SPV_KHR_float_controls2};
+
static const std::map<std::string, SPIRV::Extension::Extension, std::less<>>
SPIRVExtensionMap = {
{"SPV_EXT_shader_atomic_float_add",
@@ -104,6 +167,26 @@ static const std::map<std::string, SPIRV::Extension::Extension, std::less<>>
{"SPV_KHR_float_controls2",
SPIRV::Extension::Extension::SPV_KHR_float_controls2}};
+/*
+
+TODO: std::set_union is not constexpr in c++17. I would still like a way to make
+sure the environment lists are added.
+
+// Check that every extension is in at least one of the two lists.
+constexpr bool AreAllExtensionsInAnEnvList() noexcept {
+ SPIRV::Extension::Extension ExtensionsInEnvList[100] = {};
+ constexpr auto* end = std::set_union(ValidVulkanExtensions.begin(),
+ValidVulkanExtensions.end(), ValidOpenCLExtensions.begin(),
+ValidOpenCLExtensions.end(), ExtensionsInEnvList); return
+(end-ExtensionsInEnvList) == SPIRVExtensionMap.size();
+}
+
+static_assert(
+ AreAllExtensionsInAnEnvList(),
+ "Not all extensions are in ValidVulkanExtensions or "
+ "ValidOpenCLExtensions");
+*/
+
bool SPIRVExtensionsParser::parse(cl::Option &O, StringRef ArgName,
StringRef ArgValue,
std::set<SPIRV::Extension::Extension> &Vals) {
@@ -169,3 +252,10 @@ StringRef SPIRVExtensionsParser::checkExtensions(
}
return StringRef();
}
+
+const std::set<SPIRV::Extension::Extension> &
+SPIRVExtensionsParser::getValidExtensions(const Triple &TT) {
+ if (TT.getOS() == Triple::Vulkan)
+ return ValidVulkanExtensions;
+ return ValidOpenCLExtensions;
+}
diff --git a/llvm/lib/Target/SPIRV/SPIRVCommandLine.h b/llvm/lib/Target/SPIRV/SPIRVCommandLine.h
index 3e3b22bde8603..dbde2397ab214 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCommandLine.h
+++ b/llvm/lib/Target/SPIRV/SPIRVCommandLine.h
@@ -21,6 +21,7 @@
namespace llvm {
class StringRef;
+class Triple;
/// Command line parser for toggling SPIR-V extensions.
struct SPIRVExtensionsParser
@@ -42,6 +43,11 @@ struct SPIRVExtensionsParser
static StringRef
checkExtensions(const std::vector<std::string> &ExtNames,
std::set<SPIRV::Extension::Extension> &AllowedExtensions);
+
+ /// Returns the list of extensions that are valid for a particular
+ /// target environment (i.e., OpenCL or Vulkan).
+ static const std::set<SPIRV::Extension::Extension> &
+ getValidExtensions(const Triple &TT);
};
} // namespace llvm
diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
index cdf3c6224d4c8..690493fb426bc 100644
--- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
@@ -166,7 +166,13 @@ void SPIRVSubtarget::initAvailableExtInstSets() {
void SPIRVSubtarget::initAvailableExtensions(
const std::set<SPIRV::Extension::Extension> &AllowedExtIds) {
AvailableExtensions.clear();
- AvailableExtensions.insert_range(AllowedExtIds);
+ const std::set<SPIRV::Extension::Extension> &ValidExtensions =
+ SPIRVExtensionsParser::getValidExtensions(TargetTriple);
+
+ for (const auto &Ext : AllowedExtIds) {
+ if (ValidExtensions.count(Ext))
+ AvailableExtensions.insert(Ext);
+ }
accountForAMDShaderTrinaryMinmax();
}
diff --git a/llvm/test/CodeGen/SPIRV/extensions/enable-all-extensions-avoid-invalid.ll b/llvm/test/CodeGen/SPIRV/extensions/enable-all-extensions-avoid-invalid.ll
new file mode 100644
index 0000000000000..2de7fff0bc900
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/extensions/enable-all-extensions-avoid-invalid.ll
@@ -0,0 +1,16 @@
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv1.6-vulkan1.3-compute --spirv-ext=all %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv1.6-vulkan1.3-compute --spirv-ext=all %s -o - -filetype=obj | spirv-val --target-env vulkan1.3 %}
+
+; CHECK-NOT: OpExtension "SPV_KHR_no_integer_wrap_decoration"
+
+define internal void @foo(i32 %i) local_unnamed_addr {
+ %sub.i = sub nsw i32 0, %i
+ ret void
+}
+
+define internal void @main() local_unnamed_addr #0 {
+entry:
+ ret void
+}
+
+attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
\ No newline at end of file
|
s-perron
changed the title
Keenuts
reviewed
Jul 23, 2025
Comment on lines
+172
to
+173
| TODO: std::set_union is not constexpr in c++17. I would still like a way to make | ||
| sure the environment lists are added. |
There was a problem hiding this comment.
Could this be handled structurally by the map?
Having a map like:
struct ExtensionInfo {
const char* name;
bool valid_in_vulkan;
bool valid_in_opencl;
};
std::unorederd_map<SPIRV::Extension, ExtensionInfo> = {
}
This way we don't have to keep all 3 lists in sync?
Someone could still but the 2 valid bits to false, but it's more visible.
There was a problem hiding this comment.
Alternatively, multiclass ExtensionOperand could be extended in SPIRVSymbolicOperands.td to contain also environment requirements. This way we would not need maintain these lists here. Though, this does not need to be done in this PR.
Keenuts
reviewed
Jul 23, 2025
Naghasan
reviewed
Jul 23, 2025
| SPIRV::Extension::SPV_EXT_shader_atomic_float16_add, | ||
| SPIRV::Extension::SPV_EXT_shader_atomic_float_min_max, | ||
| SPIRV::Extension::SPV_EXT_arithmetic_fence, | ||
| SPIRV::Extension::SPV_EXT_demote_to_helper_invocation, |
There was a problem hiding this comment.
Shader specific
Suggested change
| SPIRV::Extension::SPV_EXT_demote_to_helper_invocation, |
AFAIK the rest of the list is correct.
s-perron
changed the title
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.
Not all SPIR-V extensions are allows in every environment. When we use
the
-spirv-ext=alloption, the backend currently believes that allextensions can be used.
This commit filters out the extensions on the command line to remove
those that are not known to be allowed for the current environment.
Alternatives considered: I considered modifying the
SPIRVExtensionsParser::parse to use a different list of extensions for
"all" depending on the target triple. However that does not work because
the target triple is not available, and cannot be made available in a
reasonable way.
Fixes #147717