[SPIRV] Fix disabling of default extensions - #183325
Conversation
Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
|
@llvm/pr-subscribers-backend-spir-v Author: Nick Sarnie (sarnex) ChangesIf you pass This PR makes it so disabling an extension with the The problem was we only considered the disabled extension when parsing the arguments for Full diff: https://github.com/llvm/llvm-project/pull/183325.diff 3 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp b/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
index e336cf8dbaca1..31c7a61e1a3d8 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVCommandLine.cpp
@@ -28,6 +28,8 @@
using namespace llvm;
+std::set<SPIRV::Extension::Extension> SPIRVExtensionsParser::DisabledExtensions;
+
static const std::map<StringRef, SPIRV::Extension::Extension>
SPIRVExtensionMap = {
{"SPV_EXT_shader_atomic_float_add",
@@ -231,7 +233,7 @@ bool SPIRVExtensionsParser::parse(cl::Option &O, StringRef ArgName,
return O.error(
"Extension cannot be allowed and disallowed at the same time: " +
NameValuePair->first);
-
+ DisabledExtensions.insert(NameValuePair->second);
Vals.erase(NameValuePair->second);
}
@@ -270,7 +272,8 @@ SPIRVExtensionsParser::getValidExtensions(const Triple &TT) {
SPIRV::OperandCategory::OperandCategory::ExtensionOperand,
ExtensionEnum);
- if (llvm::is_contained(AllowedEnv, CurrentEnvironment))
+ if (llvm::is_contained(AllowedEnv, CurrentEnvironment) &&
+ !llvm::is_contained(DisabledExtensions, ExtensionEnum))
R.insert(ExtensionEnum);
}
diff --git a/llvm/lib/Target/SPIRV/SPIRVCommandLine.h b/llvm/lib/Target/SPIRV/SPIRVCommandLine.h
index 02e847b322a77..03dba5915d06e 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCommandLine.h
+++ b/llvm/lib/Target/SPIRV/SPIRVCommandLine.h
@@ -48,6 +48,9 @@ struct SPIRVExtensionsParser
/// target environment (i.e., OpenCL or Vulkan).
static std::set<SPIRV::Extension::Extension>
getValidExtensions(const Triple &TT);
+
+private:
+ static std::set<SPIRV::Extension::Extension> DisabledExtensions;
};
} // namespace llvm
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_EXT_relaxed_printf_string_address_space/non-constant-printf.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_EXT_relaxed_printf_string_address_space/non-constant-printf.ll
index cdbb1605afdd1..5a330dad47aa3 100644
--- a/llvm/test/CodeGen/SPIRV/extensions/SPV_EXT_relaxed_printf_string_address_space/non-constant-printf.ll
+++ b/llvm/test/CodeGen/SPIRV/extensions/SPV_EXT_relaxed_printf_string_address_space/non-constant-printf.ll
@@ -1,6 +1,9 @@
; RUN: llc -O0 -mtriple=spirv32-unknown-unknown --spirv-ext=+SPV_EXT_relaxed_printf_string_address_space %s -o - | FileCheck %s
; RUN: llc -O0 -mtriple=spirv32-intel-unknown %s -o - | FileCheck %s
; RUN: not llc -O0 -mtriple=spirv32-unknown-unknown %s -o %t.spvt 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
+; RUN: not llc -O0 -mtriple=spirv32-intel-unknown --spirv-ext=-SPV_EXT_relaxed_printf_string_address_space %s -o %t.spvt 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
+; RUN: not llc -O0 -mtriple=spirv32-intel-unknown --spirv-ext=all,-SPV_EXT_relaxed_printf_string_address_space %s -o %t.spvt 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
+; RUN: not llc -O0 -mtriple=spirv32-intel-unknown --spirv-ext=-SPV_EXT_relaxed_printf_string_address_space,all %s -o %t.spvt 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
; CHECK: OpExtension "SPV_EXT_relaxed_printf_string_address_space"
; CHECK: %[[#ExtInstSetId:]] = OpExtInstImport "OpenCL.std"
|
|
|
||
| using namespace llvm; | ||
|
|
||
| std::set<SPIRV::Extension::Extension> SPIRVExtensionsParser::DisabledExtensions; |
There was a problem hiding this comment.
Does it need to be ordered? Would unordered_set be good enough?
There was a problem hiding this comment.
I had the same thought, but I just copied the type of the existing extensions map which is ordered. I can try changing them both to unordered if you're prefer (which makes more sense to me tbh) but I think the types should match at least
There was a problem hiding this comment.
I don't mind to change orderness later (assuming we have a github issue)
There was a problem hiding this comment.
I'm happy to change the order basically immediately after merging this PR, if approved.
There was a problem hiding this comment.
Sure, that works for me.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/39719 Here is the relevant piece of the build log for the reference |
…ensions (#183567) Review follow-up from llvm/llvm-project#183325 No reason for these data structures to be ordered. Minor annoyance when trying to use `DenseMap` because of the C++ code for enums generated by TableGen, but not too bad. Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
If you pass `-ExtName` to the `--spirv-ext` command line option. that should disable the extension. However some vendors have some extensions enabled by default when using a triple with that vendor, and disabling an extension with the option did not effect the default extensions. This PR makes it so disabling an extension with the `--spirv-ext` option actually disables the extension. The problem was we only considered the disabled extension when parsing the arguments for `--spirv-ext`, but the default extensions are added separately, so we need to store the disabled extensions and factor them in when computing the final extension set to use. Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
…vm#183567) Review follow-up from llvm#183325 No reason for these data structures to be ordered. Minor annoyance when trying to use `DenseMap` because of the C++ code for enums generated by TableGen, but not too bad. Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
…83567) Review follow-up from llvm/llvm-project#183325 No reason for these data structures to be ordered. Minor annoyance when trying to use `DenseMap` because of the C++ code for enums generated by TableGen, but not too bad. Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
If you pass
-ExtNameto the--spirv-extcommand line option. that should disable the extension. However some vendors have some extensions enabled by default when using a triple with that vendor, and disabling an extension with the option did not effect the default extensions.This PR makes it so disabling an extension with the
--spirv-extoption actually disables the extension.The problem was we only considered the disabled extension when parsing the arguments for
--spirv-ext, but the default extensions are added separately, so we need to store the disabled extensions and factor them in when computing the final extension set to use.