-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[DirectX] Validating Root flags are denying shader stage #153287
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
[DirectX] Validating Root flags are denying shader stage #153287
Conversation
@llvm/pr-subscribers-backend-directx Author: None (joaosaffran) ChangesRoot Signature Flags, allow flags to block compilation of certain shader stages. This PR implements a validation and notify the user if they compile a root signature that is denying such shader stage. Full diff: https://github.com/llvm/llvm-project/pull/153287.diff 4 Files Affected:
diff --git a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
index 3721b5f539b8c..3897056d5081a 100644
--- a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
+++ b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
@@ -21,7 +21,6 @@
#include "llvm/InitializePasses.h"
#include "llvm/MC/DXContainerRootSignature.h"
#include "llvm/Support/DXILABI.h"
-#include <cstdint>
#define DEBUG_TYPE "dxil-post-optimization-validation"
@@ -169,15 +168,16 @@ reportDescriptorTableMixingTypes(Module &M, uint32_t Location,
M.getContext().diagnose(DiagnosticInfoGeneric(Message));
}
-static void reportOverlowingRange(Module &M, const dxbc::RTS0::v2::DescriptorRange &Range) {
+static void
+reportOverlowingRange(Module &M, const dxbc::RTS0::v2::DescriptorRange &Range) {
SmallString<128> Message;
raw_svector_ostream OS(Message);
- OS << "Cannot append range with implicit lower "
- << "bound after an unbounded range "
- << getResourceClassName(toResourceClass(static_cast<dxbc::DescriptorRangeType>(Range.RangeType)))
- << "(register=" << Range.BaseShaderRegister << ", space=" <<
- Range.RegisterSpace
- << ") exceeds maximum allowed value.";
+ OS << "Cannot append range with implicit lower "
+ << "bound after an unbounded range "
+ << getResourceClassName(toResourceClass(
+ static_cast<dxbc::DescriptorRangeType>(Range.RangeType)))
+ << "(register=" << Range.BaseShaderRegister
+ << ", space=" << Range.RegisterSpace << ") exceeds maximum allowed value.";
M.getContext().diagnose(DiagnosticInfoGeneric(Message));
}
@@ -262,12 +262,57 @@ getRootDescriptorsBindingInfo(const mcdxbc::RootSignatureDesc &RSD,
return RDs;
}
+static void reportIfDeniedShaderStageAccess(Module &M, dxbc::RootFlags Flags,
+ dxbc::RootFlags Mask) {
+ if ((Flags & Mask) == Mask) {
+ SmallString<128> Message;
+ raw_svector_ostream OS(Message);
+ OS << "Shader has root bindings but root signature uses a DENY flag to "
+ "disallow root binding access to the shader stage.";
+ M.getContext().diagnose(DiagnosticInfoGeneric(Message));
+ }
+}
+
+static void validateRootFlags(Module &M, const mcdxbc::RootSignatureDesc &RSD,
+ const dxil::ModuleMetadataInfo &MMI) {
+ dxbc::RootFlags Flags = dxbc::RootFlags(RSD.Flags);
+ switch (MMI.ShaderProfile) {
+ case Triple::Pixel:
+ reportIfDeniedShaderStageAccess(M, Flags,
+ dxbc::RootFlags::DenyPixelShaderRootAccess);
+ break;
+ case Triple::Vertex:
+ reportIfDeniedShaderStageAccess(
+ M, Flags, dxbc::RootFlags::DenyVertexShaderRootAccess);
+ break;
+ case Triple::Geometry:
+ reportIfDeniedShaderStageAccess(
+ M, Flags, dxbc::RootFlags::DenyGeometryShaderRootAccess);
+ break;
+ case Triple::Hull:
+ reportIfDeniedShaderStageAccess(M, Flags,
+ dxbc::RootFlags::DenyHullShaderRootAccess);
+ break;
+ case Triple::Domain:
+ reportIfDeniedShaderStageAccess(
+ M, Flags, dxbc::RootFlags::DenyDomainShaderRootAccess);
+ break;
+ case Triple::Mesh:
+ reportIfDeniedShaderStageAccess(M, Flags,
+ dxbc::RootFlags::DenyMeshShaderRootAccess);
+ break;
+ case Triple::Amplification:
+ reportIfDeniedShaderStageAccess(
+ M, Flags, dxbc::RootFlags::DenyAmplificationShaderRootAccess);
+ break;
+ default:
+ break;
+ }
+}
static void validateDescriptorTables(Module &M,
- const mcdxbc::RootSignatureDesc &RSD,
- dxil::ModuleMetadataInfo &MMI,
- DXILResourceMap &DRM) {
+ const mcdxbc::RootSignatureDesc &RSD) {
for (const mcdxbc::RootParameterInfo &ParamInfo : RSD.ParametersContainer) {
if (static_cast<dxbc::RootParameterType>(ParamInfo.Header.ParameterType) !=
dxbc::RootParameterType::DescriptorTable)
@@ -283,30 +328,31 @@ static void validateDescriptorTables(Module &M,
uint64_t AppendingOffset = 0;
-
for (const dxbc::RTS0::v2::DescriptorRange &Range : Table.Ranges) {
dxbc::DescriptorRangeType RangeType =
static_cast<dxbc::DescriptorRangeType>(Range.RangeType);
-
+
uint64_t Offset = AppendingOffset;
- if(Range.OffsetInDescriptorsFromTableStart != ~0U)
+ if (Range.OffsetInDescriptorsFromTableStart != ~0U)
Offset = Range.OffsetInDescriptorsFromTableStart;
-
- if(Offset > ~0U)
+
+ if (Offset > ~0U)
reportOverlowingRange(M, Range);
- if(Range.NumDescriptors == ~0U) {
+ if (Range.NumDescriptors == ~0U) {
AppendingOffset = (uint64_t)~0U + (uint64_t)1ULL;
- } else {
- uint64_t UpperBound = (uint64_t)Range.BaseShaderRegister + (uint64_t)Range.NumDescriptors - (uint64_t)1U;
- if(UpperBound > ~0U)
+ } else {
+ uint64_t UpperBound = (uint64_t)Range.BaseShaderRegister +
+ (uint64_t)Range.NumDescriptors - (uint64_t)1U;
+ if (UpperBound > ~0U)
reportOverlowingRange(M, Range);
- uint64_t AppendingUpperBound = (uint64_t)Offset + (uint64_t)Range.NumDescriptors - (uint64_t)1U;
- if(AppendingUpperBound > ~0U)
+ uint64_t AppendingUpperBound =
+ (uint64_t)Offset + (uint64_t)Range.NumDescriptors - (uint64_t)1U;
+ if (AppendingUpperBound > ~0U)
reportOverlowingRange(M, Range);
AppendingOffset = Offset + Range.NumDescriptors;
}
-
+
if (RangeType == dxbc::DescriptorRangeType::Sampler) {
HasSampler = true;
} else {
@@ -441,7 +487,8 @@ static void reportErrors(Module &M, DXILResourceMap &DRM,
if (mcdxbc::RootSignatureDesc *RSD = getRootSignature(RSBI, MMI)) {
validateRootSignatureBindings(M, *RSD, MMI, DRM);
- validateDescriptorTables(M, *RSD, MMI, DRM);
+ validateDescriptorTables(M, *RSD);
+ validateRootFlags(M, *RSD, MMI);
}
}
diff --git a/llvm/test/CodeGen/DirectX/rootsignature-validation-deny-shader.ll b/llvm/test/CodeGen/DirectX/rootsignature-validation-deny-shader.ll
new file mode 100644
index 0000000000000..234909e82b792
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/rootsignature-validation-deny-shader.ll
@@ -0,0 +1,16 @@
+; RUN: opt -S -passes='dxil-post-optimization-validation' %s 2>&1
+; expected-no-diagnostics
+target triple = "dxil-pc-shadermodel6.6-geometry"
+
+define void @CSMain() #0 {
+entry:
+ ret void
+}
+attributes #0 = { noinline nounwind "exp-shader"="cs" "hlsl.numthreads"="1,2,1" "hlsl.shader"="geometry" }
+
+!dx.rootsignatures = !{!0}
+
+!0 = !{ptr @CSMain, !1, i32 2}
+!1 = !{!2}
+!2 = !{ !"RootFlags", i32 294 } ; 32 = deny_pixel/hull/vertex/amplification_shader_root_access
+
diff --git a/llvm/test/CodeGen/DirectX/rootsignature-validation-fail-deny-multiple-shader.ll b/llvm/test/CodeGen/DirectX/rootsignature-validation-fail-deny-multiple-shader.ll
new file mode 100644
index 0000000000000..9286c31db2de0
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/rootsignature-validation-fail-deny-multiple-shader.ll
@@ -0,0 +1,17 @@
+; RUN: not opt -S -passes='dxil-post-optimization-validation' %s 2>&1 | FileCheck %s
+
+; CHECK: error: Shader has root bindings but root signature uses a DENY flag to disallow root binding access to the shader stage.
+target triple = "dxil-pc-shadermodel6.6-hull"
+
+define void @CSMain() #0 {
+entry:
+ ret void
+}
+attributes #0 = { noinline nounwind "exp-shader"="cs" "hlsl.numthreads"="1,2,1" "hlsl.shader"="hull" }
+
+!dx.rootsignatures = !{!0}
+
+!0 = !{ptr @CSMain, !1, i32 2}
+!1 = !{!2}
+!2 = !{ !"RootFlags", i32 294 } ; 32 = deny_pixel/hull/vertex/amplification_shader_root_access
+
diff --git a/llvm/test/CodeGen/DirectX/rootsignature-validation-fail-deny-single-shader.ll b/llvm/test/CodeGen/DirectX/rootsignature-validation-fail-deny-single-shader.ll
new file mode 100644
index 0000000000000..7294346900415
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/rootsignature-validation-fail-deny-single-shader.ll
@@ -0,0 +1,17 @@
+; RUN: not opt -S -passes='dxil-post-optimization-validation' %s 2>&1 | FileCheck %s
+
+; CHECK: error: Shader has root bindings but root signature uses a DENY flag to disallow root binding access to the shader stage.
+target triple = "dxil-pc-shadermodel6.6-pixel"
+
+define void @CSMain() #0 {
+entry:
+ ret void
+}
+attributes #0 = { noinline nounwind "exp-shader"="cs" "hlsl.numthreads"="1,2,1" "hlsl.shader"="pixel" }
+
+!dx.rootsignatures = !{!0}
+
+!0 = !{ptr @CSMain, !1, i32 2}
+!1 = !{!2}
+!2 = !{ !"RootFlags", i32 32 } ; 32 = deny_pixel_shader_root_access
+
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
if (RK != ResourceKind::RawBuffer && | ||
RK != ResourceKind::StructuredBuffer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatter trigger that, since it gets linger than 80 columns
Builder.findOverlapping(ReportedBinding); | ||
reportOverlappingRegisters(M, ReportedBinding, Overlaping); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is intentional, I think it improves readability.
if ((Flags & Mask) != Mask) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this check here, because I think it simplifies the code. If this is not here, I need to make the same check in all statements of the switch with slightly different masks.
@@ -0,0 +1,20 @@ | |||
; RUN: not opt -S -passes='dxil-post-optimization-validation' %s 2>&1 | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test checks an edge case, where root descriptors can be bound with non textures, but if they are denied, it should fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, just some clean ups I think we should do
} | ||
|
||
if (HasBindings && MMI.ShaderProfile != Triple::Compute) { | ||
dxbc::RootFlags Flags = dxbc::RootFlags(RSD.Flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it may be nicer if we define a separate function like getEnvironmentDenyFlag
and then just invoked reportIfDeniedShaderStageAccess
once. Might prevent a copy of this switch table appearing elsewhere
if (HasBindings && MMI.ShaderProfile != Triple::Compute) { | ||
const dxbc::RootFlags Flags = dxbc::RootFlags(RSD.Flags); | ||
const dxbc::RootFlags Mask = getEnvironmentDenyFlagMask(MMI.ShaderProfile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this do the right thing for Library shaders? What about the various raytracing profiles (RayGeneration, Intersection, AnyHit, etc)? Is there some reason we can't get here for those?
ParamInfo->Type == dxbc::RootParameterType::DescriptorTable; | ||
const bool IsRawOrStructuredBuffer = | ||
RK != ResourceKind::RawBuffer && RK != ResourceKind::StructuredBuffer; | ||
if (IsRootSRVOrUAV && !IsDescriptorTable && IsRawOrStructuredBuffer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the !IsDescriptorTable
needed? I would've assumed that IsRootSRVOrUAV
would imply that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsRootSRVOrUAV
only check if the resource being bound is SRV
or UAV
, is not checking what kind of resource is being bounded to. In this validation, you can bound to a DescriptorTable
but not with a RootSRV
or RootUAV
.
Here is a godbolt link with this example:
https://hlsl.godbolt.org/z/WW4qTs4hf
llvm/test/CodeGen/DirectX/rootsignature-validation-fail-deny-multiple-shader.ll
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good. A few minor comments yet.
const std::optional<dxbc::RootFlags> Mask = | ||
getEnvironmentDenyFlagMask(MMI.ShaderProfile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it isn't wrong, const
on variables with value types like bool
and std::optional
doesn't gain us a lot (since the memory is local anyway) so it's somewhat uncommon. I would probably not bother with them here as a matter of style and clarity.
if (!Mask.has_value() || !HasBindings) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's slightly clearer to check !HasBindings
and return early before calling getEnvironmentalDenyFlagMask
. This makes it obvious that the Mask doesn't matter if we don't have any bindings. This could also let you nest the declaration of Mask
in the if ()
condition if you think it reads better:
if (std::optional<dxbc::RootFlags> Mask =
getEnvironmentDenyFlagMask(MMI.ShaderProfile))
reportIfDeniedShaderStageAccess(M, dxbc::RootFlags(RSD.Flags), *Mask);
if (!Mask.has_value() || !HasBindings) | ||
return; | ||
|
||
const dxbc::RootFlags Flags = dxbc::RootFlags(RSD.Flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know that RSD.Flags
is valid at this point and that's why this cast is okay, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, RSD.Flags
is validated when we read from the metadata, this data is not changed after such validation, so is valid.
d7dd8f0
into
llvm:users/joaosaffran/153276
Root Signature Flags, allow flags to block compilation of certain shader stages. This PR implements a validation and notify the user if they compile a root signature that is denying such shader stage. Closes: #153062 Previously approved: #153287 --------- Co-authored-by: joaosaffran <[email protected]> Co-authored-by: Joao Saffran <{ID}+{username}@users.noreply.github.com> Co-authored-by: Joao Saffran <[email protected]>
…#160919) Root Signature Flags, allow flags to block compilation of certain shader stages. This PR implements a validation and notify the user if they compile a root signature that is denying such shader stage. Closes: llvm/llvm-project#153062 Previously approved: llvm/llvm-project#153287 --------- Co-authored-by: joaosaffran <[email protected]> Co-authored-by: Joao Saffran <{ID}+{username}@users.noreply.github.com> Co-authored-by: Joao Saffran <[email protected]>
Root Signature Flags, allow flags to block compilation of certain shader stages. This PR implements a validation and notify the user if they compile a root signature that is denying such shader stage. Closes: llvm#153062 Previously approved: llvm#153287 --------- Co-authored-by: joaosaffran <[email protected]> Co-authored-by: Joao Saffran <{ID}+{username}@users.noreply.github.com> Co-authored-by: Joao Saffran <[email protected]>
Root Signature Flags, allow flags to block compilation of certain shader stages. This PR implements a validation and notify the user if they compile a root signature that is denying such shader stage.
Closes: #153062