-
Notifications
You must be signed in to change notification settings - Fork 17k
[SPIR-V] Fix environment resolution causing legalization crash #179052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
602a287
bf7bc9d
54d67d9
6cc7d39
881c120
048775b
222c6ac
ec15a9e
a245a7b
8e21f07
7f5d49c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,25 @@ void SPIRVSubtarget::initAvailableExtInstSets() { | |
| accountForAMDShaderTrinaryMinmax(); | ||
| } | ||
|
|
||
| void SPIRVSubtarget::resolveEnvFromModule(const Module &M) { | ||
| if (Env != Unknown) | ||
| return; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| bool HasShaderAttr = false; | ||
| for (const Function &F : M) { | ||
| if (F.getFnAttribute("hlsl.shader").isValid()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just I'm not sure if the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 . |
||
| 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) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(). | ||
|
MrSidims marked this conversation as resolved.
Outdated
|
||
| 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; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I've made resolveEnvFromModule calling setEnv and also moved initAvailableExtInstSets and set of legalizer info to the setEnv |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 %} | ||
|
MrSidims marked this conversation as resolved.
Outdated
|
||
|
|
||
| ; 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is the use of the wide vector?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ehhh, how I ended up with this test? Some odd script put in llvm-reduce script may be? Thanks for spotting! |
||
| ; 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" | ||
|
MrSidims marked this conversation as resolved.
|
||
|
|
||
| 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" } | ||
Uh oh!
There was an error while loading. Please reload this page.