[SPIR-V] Fix environment resolution causing legalization crash#179052
Merged
[SPIR-V] Fix environment resolution causing legalization crash#179052
Conversation
When the triple is spirv-unknown-unknown, SPIRVSubtarget::Env starts as Unknown and was set via const_cast in SPIRVCallLowering when the first entry point was lowered. This is too late: SPIRVLegalizerInfo has already been constructed with, for example, the wrong vector size limits, causing a crash (at best) or invalid SPIR-V generation. Resolve the environment early in SPIRVPrepareFunctions::runOnModule() by scanning the module for "hlsl.shader" attributes. Reinitialize the legalizer and extended instruction sets after resolution. Remove the const_cast lazy setting from SPIRVCallLowering.
MrSidims
requested review from
Keenuts,
jmmartinez,
maarquitos14,
michalpaszkowski and
vmaksimo
Member
|
@llvm/pr-subscribers-backend-spir-v Author: Dmitry Sidorov (MrSidims) ChangesWhen the triple is spirv-unknown-unknown, SPIRVSubtarget::Env starts as Unknown and was set via const_cast in SPIRVCallLowering when the first entry point was lowered. This is too late: SPIRVLegalizerInfo has already been constructed with, for example, the wrong vector size limits, causing a crash (at best) or invalid SPIR-V generation. Resolve the environment early in SPIRVPrepareFunctions::runOnModule() by scanning the module for "hlsl.shader" attributes. Reinitialize the legalizer and extended instruction sets after resolution. Remove the const_cast lazy setting from SPIRVCallLowering. Fixes: #171898 Full diff: https://github.com/llvm/llvm-project/pull/179052.diff 6 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp b/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
index 23c5798f9d0af..dff9b4a48564b 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
@@ -221,41 +221,17 @@ static SPIRVType *getArgSPIRVType(const Function &F, unsigned ArgIdx,
static SPIRV::ExecutionModel::ExecutionModel
getExecutionModel(const SPIRVSubtarget &STI, const Function &F) {
+ assert(STI.getEnv() != SPIRVSubtarget::Unknown &&
+ "Environment must be resolved before lowering entry points. ");
+
if (STI.isKernel())
return SPIRV::ExecutionModel::Kernel;
- if (STI.isShader()) {
- auto attribute = F.getFnAttribute("hlsl.shader");
- if (!attribute.isValid()) {
- report_fatal_error(
- "This entry point lacks mandatory hlsl.shader attribute.");
- }
-
- const auto value = attribute.getValueAsString();
- if (value == "compute")
- return SPIRV::ExecutionModel::GLCompute;
- if (value == "vertex")
- return SPIRV::ExecutionModel::Vertex;
- if (value == "pixel")
- return SPIRV::ExecutionModel::Fragment;
-
- report_fatal_error(
- "This HLSL entry point is not supported by this backend.");
- }
-
- assert(STI.getEnv() == SPIRVSubtarget::Unknown);
- // "hlsl.shader" attribute is mandatory for Vulkan, so we can set Env to
- // Shader whenever we find it, and to Kernel otherwise.
-
- // We will now change the Env based on the attribute, so we need to strip
- // `const` out of the ref to STI.
- SPIRVSubtarget *NonConstSTI = const_cast<SPIRVSubtarget *>(&STI);
auto attribute = F.getFnAttribute("hlsl.shader");
if (!attribute.isValid()) {
- NonConstSTI->setEnv(SPIRVSubtarget::Kernel);
- return SPIRV::ExecutionModel::Kernel;
+ report_fatal_error(
+ "This entry point lacks mandatory hlsl.shader attribute.");
}
- NonConstSTI->setEnv(SPIRVSubtarget::Shader);
const auto value = attribute.getValueAsString();
if (value == "compute")
@@ -432,11 +408,6 @@ bool SPIRVCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
// Handle entry points and function linkage.
if (isEntryPoint(F)) {
- // EntryPoints can help us to determine the environment we're working on.
- // Therefore, we need a non-const pointer to SPIRVSubtarget to update the
- // environment if we need to.
- const SPIRVSubtarget *ST =
- static_cast<const SPIRVSubtarget *>(&MIRBuilder.getMF().getSubtarget());
auto MIB = MIRBuilder.buildInstr(SPIRV::OpEntryPoint)
.addImm(static_cast<uint32_t>(getExecutionModel(*ST, F)))
.addUse(FuncVReg);
diff --git a/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp b/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
index a3425704f050d..1aedab4e94048 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp
@@ -656,6 +656,12 @@ bool SPIRVPrepareFunctions::removeAggregateTypesFromCalls(Function *F) {
}
bool SPIRVPrepareFunctions::runOnModule(Module &M) {
+ // Resolve the SPIR-V environment from module content before any
+ // function-level processing. This must happen before legalization so that
+ // isShader()/isKernel() return correct values.
+ const_cast<SPIRVTargetMachine &>(TM).getMutableSubtargetImpl()
+ ->resolveEnvFromModule(M);
+
bool Changed = false;
for (Function &F : M) {
Changed |= substituteIntrinsicCalls(&F);
diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
index ad6c9cd421b7c..7e1a41dcb2276 100644
--- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
@@ -173,6 +173,25 @@ void SPIRVSubtarget::initAvailableExtInstSets() {
accountForAMDShaderTrinaryMinmax();
}
+void SPIRVSubtarget::resolveEnvFromModule(const Module &M) {
+ if (Env != Unknown)
+ return;
+
+ bool HasShaderAttr = false;
+ for (const Function &F : M) {
+ if (F.getFnAttribute("hlsl.shader").isValid()) {
+ HasShaderAttr = true;
+ break;
+ }
+ }
+
+ Env = HasShaderAttr ? Shader : Kernel;
+
+ // Reinitialize Env-dependent state: ext inst sets and legalizer info.
+ initAvailableExtInstSets();
+ Legalizer = std::make_unique<SPIRVLegalizerInfo>(*this);
+}
+
// Set available extensions after SPIRVSubtarget is created.
void SPIRVSubtarget::initAvailableExtensions(
const std::set<SPIRV::Extension::Extension> &AllowedExtIds) {
diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.h b/llvm/lib/Target/SPIRV/SPIRVSubtarget.h
index ad3e38d296ed7..01888adf88fbb 100644
--- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.h
+++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.h
@@ -25,6 +25,7 @@
#include "llvm/CodeGen/SelectionDAGTargetInfo.h"
#include "llvm/CodeGen/TargetSubtargetInfo.h"
#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/Module.h"
#include "llvm/Target/TargetMachine.h"
#include "llvm/TargetParser/Triple.h"
@@ -62,9 +63,6 @@ class SPIRVSubtarget : public SPIRVGenSubtargetInfo {
std::unique_ptr<InstructionSelector> InstSelector;
std::unique_ptr<InlineAsmLowering> InlineAsmInfo;
- // TODO: Initialise the available extensions, extended instruction sets
- // based on the environment settings.
- void initAvailableExtInstSets();
void accountForAMDShaderTrinaryMinmax();
public:
@@ -76,6 +74,11 @@ class SPIRVSubtarget : public SPIRVGenSubtargetInfo {
void initAvailableExtensions(
const std::set<SPIRV::Extension::Extension> &AllowedExtIds);
+ void initAvailableExtInstSets();
+
+ // If Env is Unknown, scan module for "hlsl.shader" attributes to resolve it.
+ // Must be called before any pass that depends on isShader()/isKernel().
+ void resolveEnvFromModule(const Module &M);
// Parses features string setting specified subtarget options.
// The definition of this function is auto generated by tblgen.
@@ -86,8 +89,8 @@ class SPIRVSubtarget : public SPIRVGenSubtargetInfo {
void setEnv(SPIRVEnvType E) {
if (E == Unknown)
report_fatal_error("Unknown environment is not allowed.");
- if (Env != Unknown)
- report_fatal_error("Environment is already set.");
+ if (Env != Unknown && Env != E)
+ report_fatal_error("Environment is already set to a different value.");
Env = E;
}
diff --git a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.h b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.h
index 9c59d021dfc1b..ea09fe98c55ee 100644
--- a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.h
+++ b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.h
@@ -35,6 +35,8 @@ class SPIRVTargetMachine : public CodeGenTargetMachineImpl {
return &Subtarget;
}
+ SPIRVSubtarget *getMutableSubtargetImpl() { return &Subtarget; }
+
TargetTransformInfo getTargetTransformInfo(const Function &F) const override;
TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
diff --git a/llvm/test/CodeGen/SPIRV/is-shader-env.ll b/llvm/test/CodeGen/SPIRV/is-shader-env.ll
new file mode 100644
index 0000000000000..47a323ded0df5
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/is-shader-env.ll
@@ -0,0 +1,25 @@
+; RUN: llc -O0 -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val %}
+
+; Regression test for https://github.com/llvm/llvm-project/issues/171898
+; When triple is spirv-unknown-unknown and a non-entry-point function using
+; wide vectors (e.g. <8 x i32>) appears before the entry point with
+; hlsl.shader attribute, the environment must be resolved early enough that
+; legalization uses the correct vector size limits.
+
+; CHECK-DAG: OpCapability Shader
+; CHECK-DAG: OpEntryPoint GLCompute %[[#entry:]] "main"
+
+define <4 x i32> @helper(<4 x i32> %a, <4 x i32> %b) {
+entry:
+ %result = add <4 x i32> %a, %b
+ ret <4 x i32> %result
+}
+
+define void @main() #0 {
+entry:
+ %a = call <4 x i32> @helper(<4 x i32> <i32 1, i32 2, i32 3, i32 4>, <4 x i32> <i32 5, i32 6, i32 7, i32 8>)
+ ret void
+}
+
+attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
maarquitos14
approved these changes
Feb 2, 2026
Contributor
maarquitos14
left a comment
maarquitos14
left a comment
There was a problem hiding this comment.
LGTM. Just one question: do we expect this to have any impact in performance? We're now going through functions once more.
jmmartinez
reviewed
Feb 2, 2026
|
|
||
| bool HasShaderAttr = false; | ||
| for (const Function &F : M) { | ||
| if (F.getFnAttribute("hlsl.shader").isValid()) { |
Contributor
There was a problem hiding this comment.
Why not just hasFnAttribute?
I'm not sure if the hlsl.shader validity is enforced through the Verifier, but we could.
Contributor
There was a problem hiding this comment.
Just for the context, shouldn't the caller be passing the appropriate target triple (maybe we do not have the right choices and we need to add one more)?
Contributor
Author
There was a problem hiding this comment.
Why not just hasFnAttribute
My reading is, that the attribute has a string parameter which may or may not be valid. On the other hand, it might be an unnecessary check as it certainly should be checked elsewhere.
Just for the context, shouldn't the caller be passing the appropriate target triple (maybe we do not have the right choices and we need to add one more)?
You mean parent triple, right? Then the question would fall to: "what environment to set in case of unknown-unknown-unknown", which previous logic was trying to address.
Contributor
Author
There was a problem hiding this comment.
Well, we could force frontend to pick between vulkan and opencl, but who knows, how many spir/spirv producers are there in the wild. and unknown-unknown was kinda a standard for a long time :)
Contributor
There was a problem hiding this comment.
Thanks for the clarification.
Maybe its something to define better in the future about what "unknown" means (silently inferring shader capabilities sound like a footgun).
Contributor
Author
There was a problem hiding this comment.
We may discuss it on the next backend WG meeting (if it wasn't discussed in the past already). FYI @michalpaszkowski . IMHO we must align on and merge #170297 before deprecating unknown-unknown .
Contributor
Author
Not something, we should care about, as we iterate only over functions, not instructions. |
jmmartinez
approved these changes
Feb 2, 2026
vmaksimo
reviewed
Feb 3, 2026
Contributor
vmaksimo
left a comment
vmaksimo
left a comment
There was a problem hiding this comment.
LGTM, just a small comment-style suggestion
s-perron
reviewed
Feb 3, 2026
Contributor
s-perron
left a comment
s-perron
left a comment
There was a problem hiding this comment.
Thanks for fixing this up. This seems like it is early enough that we should not run into problems. I just have a few suggestions to improve the safety and the test.
Comment on lines
87
to
94
| void setEnv(SPIRVEnvType E) { | ||
| if (E == Unknown) | ||
| report_fatal_error("Unknown environment is not allowed."); | ||
| if (Env != Unknown) | ||
| report_fatal_error("Environment is already set."); | ||
| if (Env != Unknown && Env != E) | ||
| report_fatal_error("Environment is already set to a different value."); | ||
|
|
||
| Env = E; | ||
| } |
Contributor
There was a problem hiding this comment.
Is this function called anymore? I think the two calls to the function were removed, and we could simply remove it?
It creates a problem. If someone were to call it the instruction sets would not be reinitialized properly. If we keep it we need to fix that up.
Contributor
There was a problem hiding this comment.
Now that I think about it more. Can you have this function call initAvailableExtInstSets and reinitialize the legalizer. Then both places that set Env should call this function. Having 1 place that sets Env could help if we find other things that need to be reset based on the env.
Contributor
Author
There was a problem hiding this comment.
Sure, I've made resolveEnvFromModule calling setEnv and also moved initAvailableExtInstSets and set of legalizer info to the setEnv
|
|
||
| ; Regression test for https://github.com/llvm/llvm-project/issues/171898 | ||
| ; When triple is spirv-unknown-unknown and a non-entry-point function using | ||
| ; wide vectors (e.g. <8 x i32>) appears before the entry point with |
Contributor
There was a problem hiding this comment.
Where is the use of the wide vector? @helper uses on 4xi32.
Contributor
Author
There was a problem hiding this comment.
Ehhh, how I ended up with this test? Some odd script put in llvm-reduce script may be? Thanks for spotting!
Comment on lines
+177
to
+178
| if (Env != Unknown) | ||
| return; |
Contributor
There was a problem hiding this comment.
Do we care about the case where the same instance of the backend is used to compile multiple modules that may have different ENVs? I think it is safe to fail in that case because that is generally not supported by an LLVM module anyway. I just want to be sure we make an explicit decision.
Contributor
Author
There was a problem hiding this comment.
I don't think we should care, but probably it's a good idea to add an assertion to document in the code our expectations.
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) lldb-apilldb-api.tools/lldb-dap/stopped-events/TestDAP_stopped_events.pyIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
s-perron
approved these changes
Feb 4, 2026
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/34208 Here is the relevant piece of the build log for the reference |
rishabhmadan19
pushed a commit
to rishabhmadan19/llvm-project
that referenced
this pull request
Feb 9, 2026
…179052) When the triple is spirv-unknown-unknown, SPIRVSubtarget::Env starts as Unknown and was set via const_cast in SPIRVCallLowering when the first entry point was lowered. This is too late: SPIRVLegalizerInfo has already been constructed with, for example, the wrong vector size limits, causing a crash (at best) or invalid SPIR-V generation. Resolve the environment early in SPIRVPrepareFunctions::runOnModule() by scanning the module for "hlsl.shader" attributes. Reinitialize the legalizer and extended instruction sets after resolution. Remove the const_cast lazy setting from SPIRVCallLowering. Fixes: llvm#171898
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
When the triple is spirv-unknown-unknown, SPIRVSubtarget::Env starts as Unknown and was set via const_cast in SPIRVCallLowering when the first entry point was lowered. This is too late: SPIRVLegalizerInfo has already been constructed with, for example, the wrong vector size limits, causing a crash (at best) or invalid SPIR-V generation.
Resolve the environment early in SPIRVPrepareFunctions::runOnModule() by scanning the module for "hlsl.shader" attributes. Reinitialize the legalizer and extended instruction sets after resolution. Remove the const_cast lazy setting from SPIRVCallLowering.
Fixes: #171898