[DirectX] Set shader feature flags MinimumPrecision and NativeLowPrecision, and refactor the logic for setting low-precision-related flags #139623
Conversation
|
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-backend-directx Author: Deric C. (Icohedron) ChangesFixes #138997. The shader feature flags MinimumPrecision and NativeLowPrecision were not being set, leading to validation errors. Full diff: https://github.com/llvm/llvm-project/pull/139623.diff 4 Files Affected:
diff --git a/llvm/lib/Target/DirectX/DXILShaderFlags.cpp b/llvm/lib/Target/DirectX/DXILShaderFlags.cpp
index b50a9b5d6051c..36f909ffa6ef5 100644
--- a/llvm/lib/Target/DirectX/DXILShaderFlags.cpp
+++ b/llvm/lib/Target/DirectX/DXILShaderFlags.cpp
@@ -296,6 +296,14 @@ void ModuleShaderFlags::initialize(Module &M, DXILResourceTypeMap &DRTM,
if (NumUAVs > 8)
CombinedSFMask.Max64UAVs = true;
+ // Set the Shader Feature Info flags related to low-precision datatypes
+ if (CombinedSFMask.LowPrecisionPresent) {
+ if (CombinedSFMask.UseNativeLowPrecision)
+ CombinedSFMask.NativeLowPrecision = true;
+ else
+ CombinedSFMask.MinimumPrecision = true;
+ }
+
CombinedSFMask.UAVsAtEveryStage = hasUAVsAtEveryStage(DRM, MMDI);
}
diff --git a/llvm/test/CodeGen/DirectX/ShaderFlags/low-precision.ll b/llvm/test/CodeGen/DirectX/ShaderFlags/low-precision.ll
index 561e09bb1e9dc..bccc14d2b38cb 100644
--- a/llvm/test/CodeGen/DirectX/ShaderFlags/low-precision.ll
+++ b/llvm/test/CodeGen/DirectX/ShaderFlags/low-precision.ll
@@ -1,4 +1,10 @@
; RUN: opt -S --passes="print-dx-shader-flags" 2>&1 %s | FileCheck %s
+; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC
+
+; Check that when the dx.nativelowprec module flag is not specified, the
+; module-level shader flag UseNativeLowPrecision is not set, and the
+; MinimumPrecision feature flag is set due to the presence of half and i16
+; without native low precision.
target triple = "dxil-pc-shadermodel6.7-library"
@@ -6,25 +12,33 @@ target triple = "dxil-pc-shadermodel6.7-library"
;CHECK-NEXT: ; Shader Flags Value: 0x00000020
;CHECK-NEXT: ;
;CHECK-NEXT: ; Note: shader requires additional functionality:
+;CHECK-NEXT: ; Minimum-precision data types
;CHECK-NEXT: ; Note: extra DXIL module flags:
;CHECK-NEXT: ; Low-precision data types
;CHECK-NEXT: ;
;CHECK-NEXT: ; Shader Flags for Module Functions
;CHECK-LABEL: ; Function add_i16 : 0x00000020
-define i16 @add_i16(i16 %a, i16 %b) {
+define i16 @add_i16(i16 %a, i16 %b) "hlsl.export" {
%sum = add i16 %a, %b
ret i16 %sum
}
;CHECK-LABEL: ; Function add_i32 : 0x00000000
-define i32 @add_i32(i32 %a, i32 %b) {
+define i32 @add_i32(i32 %a, i32 %b) "hlsl.export" {
%sum = add i32 %a, %b
ret i32 %sum
}
;CHECK-LABEL: ; Function add_half : 0x00000020
-define half @add_half(half %a, half %b) {
+define half @add_half(half %a, half %b) "hlsl.export" {
%sum = fadd half %a, %b
ret half %sum
}
+
+; DXC: - Name: SFI0
+; DXC-NEXT: Size: 8
+; DXC-NEXT: Flags:
+; DXC: MinimumPrecision: true
+; DXC: NativeLowPrecision: false
+; DXC: ...
diff --git a/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-0.ll b/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-0.ll
index c537a01482f39..e4d55d3d89013 100644
--- a/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-0.ll
+++ b/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-0.ll
@@ -1,7 +1,9 @@
; RUN: opt -S --passes="print-dx-shader-flags" 2>&1 %s | FileCheck %s
+; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC
; Check that when the dx.nativelowprec module flag is set to 0, the module-level
-; shader flag UseNativeLowPrecision is not set
+; shader flag UseNativeLowPrecision is not set, and the MinimumPrecision feature
+; flag is set due to the presence of half and i16 without native low precision.
target triple = "dxil-pc-shadermodel6.7-library"
@@ -9,6 +11,7 @@ target triple = "dxil-pc-shadermodel6.7-library"
;CHECK-NEXT: ; Shader Flags Value: 0x00000020
;CHECK-NEXT: ;
;CHECK-NEXT: ; Note: shader requires additional functionality:
+;CHECK-NEXT: ; Minimum-precision data types
;CHECK-NEXT: ; Note: extra DXIL module flags:
;CHECK-NEXT: ; Low-precision data types
;CHECK-NOT: ; Native 16bit types enabled
@@ -16,22 +19,29 @@ target triple = "dxil-pc-shadermodel6.7-library"
;CHECK-NEXT: ; Shader Flags for Module Functions
;CHECK-LABEL: ; Function add_i16 : 0x00000020
-define i16 @add_i16(i16 %a, i16 %b) {
+define i16 @add_i16(i16 %a, i16 %b) "hlsl.export" {
%sum = add i16 %a, %b
ret i16 %sum
}
;CHECK-LABEL: ; Function add_i32 : 0x00000000
-define i32 @add_i32(i32 %a, i32 %b) {
+define i32 @add_i32(i32 %a, i32 %b) "hlsl.export" {
%sum = add i32 %a, %b
ret i32 %sum
}
;CHECK-LABEL: ; Function add_half : 0x00000020
-define half @add_half(half %a, half %b) {
+define half @add_half(half %a, half %b) "hlsl.export" {
%sum = fadd half %a, %b
ret half %sum
}
!llvm.module.flags = !{!0}
!0 = !{i32 1, !"dx.nativelowprec", i32 0}
+
+; DXC: - Name: SFI0
+; DXC-NEXT: Size: 8
+; DXC-NEXT: Flags:
+; DXC: MinimumPrecision: true
+; DXC: NativeLowPrecision: false
+; DXC: ...
diff --git a/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-1.ll b/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-1.ll
index 07c4b9064d666..6ee592de087a6 100644
--- a/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-1.ll
+++ b/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-1.ll
@@ -1,4 +1,9 @@
; RUN: opt -S --passes="print-dx-shader-flags" 2>&1 %s | FileCheck %s
+; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC
+
+; Check that when the dx.nativelowprec module flag is set to 1, the module-level
+; shader flag UseNativeLowPrecision is set, and the NativeLowPrecision feature
+; flag is set
target triple = "dxil-pc-shadermodel6.7-library"
@@ -6,6 +11,7 @@ target triple = "dxil-pc-shadermodel6.7-library"
;CHECK-NEXT: ; Shader Flags Value: 0x00800020
;CHECK-NEXT: ;
;CHECK-NEXT: ; Note: shader requires additional functionality:
+;CHECK-NEXT: ; Use native low precision
;CHECK-NEXT: ; Note: extra DXIL module flags:
;CHECK-NEXT: ; Low-precision data types
;CHECK-NEXT: ; Use native low precision
@@ -13,7 +19,7 @@ target triple = "dxil-pc-shadermodel6.7-library"
;CHECK-NEXT: ; Shader Flags for Module Functions
;CHECK-LABEL: ; Function add_i16 : 0x00800020
-define i16 @add_i16(i16 %a, i16 %b) {
+define i16 @add_i16(i16 %a, i16 %b) "hlsl.export" {
%sum = add i16 %a, %b
ret i16 %sum
}
@@ -22,16 +28,23 @@ define i16 @add_i16(i16 %a, i16 %b) {
; in the module regardless of whether or not the function uses low precision
; data types (flag 0x20). This matches the behavior in DXC
;CHECK-LABEL: ; Function add_i32 : 0x00800000
-define i32 @add_i32(i32 %a, i32 %b) {
+define i32 @add_i32(i32 %a, i32 %b) "hlsl.export" {
%sum = add i32 %a, %b
ret i32 %sum
}
;CHECK-LABEL: ; Function add_half : 0x00800020
-define half @add_half(half %a, half %b) {
+define half @add_half(half %a, half %b) "hlsl.export" {
%sum = fadd half %a, %b
ret half %sum
}
!llvm.module.flags = !{!0}
!0 = !{i32 1, !"dx.nativelowprec", i32 1}
+
+; DXC: - Name: SFI0
+; DXC-NEXT: Size: 8
+; DXC-NEXT: Flags:
+; DXC: MinimumPrecision: false
+; DXC: NativeLowPrecision: true
+; DXC: ...
|
|
I think we need to refactor / clean up the handling of the MinimumPrecision/NativeLowPrecision and LowPrecisionPresent/UseNativeLowPrecision flags. This code is starting to become very hard to follow.
For (1), it does look like we need MinimumPrecision, NativeLowPrecision, and LowPrecisionPresent. LowPrecisionPresent seems to be set in the container regardless of whether 16 bit types are enabled so I don't think it can be folded in to the other two. UseNativeLowPrecision on the other hand feels redundant to me. The one place that's tricky here is related to a comment in use-native-low-precision-1.ll - if we set the function flag purely based on the compiler flag but the container flag depends on actual use, then they do need to be separate. Is this done by dxc intentionally or is it a bug? It seems weird to have a flag that says there are 16 bit types when there aren't any present. For (2), I think we should move the logic that reads the module metadata out of the loop and precalculate a bool to look it up rather than calculate it every time, and then put all of the logic to set these correctly where we handle LowPrecisionPresent today. IIUC The module flags aren't involved in the function flag calculation, so it should be fine to set those here, and that way we can have one place that has all of this logic and can be commented appropriately to explain what's happening. |
Good points. I will refactor the logic for these shader flags to hoist the reading of the dx.nativelowprec module flag out of loops and consolidate the code for setting MinimumPrecision, NativeLowPrecision, and LowPrecisionPresent into where LowPrecisionPresent is currently being set to make the code more readable. I do agree that having two similar but subtly different flags UseNativeLowPrecision and NativeLowPrecision is rather confusing. Perhaps UseNativeLowPrecision could be renamed to NativeLowPrecisionAvailable, and NativeLowPrecision could be renamed to UsesNativeLowPrecision? The former to indicate availability of native low-precision data types because they have been enabled via a command-line flag / module flag (and the hardware supports it), and the latter to indicate the actual use of native low-precision data types? Regarding the comment in use-native-low-precision-1.ll and whether or not it is a bug in DXC: I think it probably doesn't matter if it is a bug in DXC. It would probably be fine to set UseNativeLowPrecision only when there are low-precision data types present in the module. If a module doesn't use low-precision data types, then whether or not the UseNativeLowPrecision flag is set should not matter -- it shouldn't change the semantics of the shader, and it should still pass the Validator. This would need verification/testing, but if this works then the UseNativeLowPrecision and the NativeLowPrecision could be combined into a single SHADER_FEATURE_FLAG entry in DXContainerConstants.def. |
…tiveLowPrecision module and feature flags
| // is needed even if the module does not use 16-bit types because a | ||
| // corresponding debug module may include 16-bit types, and tools that use the | ||
| // debug module may expect it to have the same flags as the original | ||
| CombinedSFMask.NativeLowPrecisionMode = NativeLowPrecisionMode; |
There was a problem hiding this comment.
Setting the module flags via CombinedSFMask instead of SCCSF will make the module flags be set on the module only and not the functions themselves. This is consistent with the way the DisableOptimizations flag is currently set in Clang.
However, it does not match the behavior of DXC (see DxilShaderFlags.cpp) which sets every flag at the function level.
I don't think it matters though? The final DXIL module flags and shader feature flags work out to be the same in the end.
There was a problem hiding this comment.
Yeah, this feels like an implementation detail - I don't think there's any way to see a difference in the end.
|
The refactoring is complete and the PR is ready for re-review.
|
Icohedron
changed the title
| // is needed even if the module does not use 16-bit types because a | ||
| // corresponding debug module may include 16-bit types, and tools that use the | ||
| // debug module may expect it to have the same flags as the original | ||
| CombinedSFMask.NativeLowPrecisionMode = NativeLowPrecisionMode; |
There was a problem hiding this comment.
Yeah, this feels like an implementation detail - I don't think there's any way to see a difference in the end.
| bool CanSetResMayNotAlias; // dx.resmayalias | ||
| bool NativeLowPrecisionMode; // dx.nativelowprec |
There was a problem hiding this comment.
Should we name these similarly to reflect their similar usage/purpose? Does CanSetNativeLowPrecision make sense? If that feels forced I think HasResMayAlias and HasNativeLowPrec wouldn't be bad, though that would require flipping the value of the existing member.
There was a problem hiding this comment.
I think CanSetNativeLowPrecisionMode would make more sense
| ; DXIL Validator Version < 1.8. The ResMayNotAlias module flag (0x20000000) | ||
| ; should be set if there are one or more UAVs present globally in the |
There was a problem hiding this comment.
Should we add an llc | obj2yaml run line to this test like we have in low-precision.ll?
There was a problem hiding this comment.
There is nothing to test in the obj2yaml because ResMayNotAlias is only a module flag. It doesn't have a related shader feature flag that will show up in SFI0
Co-authored-by: Justin Bogner <[email protected]>
Fixes #138997 and does refactoring for low-precision-related flags.
The shader feature flags MinimumPrecision and NativeLowPrecision were not being set, leading to validation errors.
This PR sets these shader feature flags as in DXC and adds tests for them.
This PR also performs some refactoring of low-precision-related flags to make it less confusing.
UseNativeLowPrecisionDXIL module flag has been renamed toNativeLowPrecisionModeto imply that it is setting some execution state which the module should be interpreted withdx.nativelowprecis now read only once and sets a bool to be used byupdateFunctionFlags()and for setting the DXIL module flagNativeLowPrecisionModeMinimumPrecision,NativeLowPrecision, andLowPrecisionPresentshader feature flags are all set together underupdateFunctionFlags()NativeLowPrecisionModeandResMayNotAliasout of the per-function loop and placed it alongside the logic for setting other DXIL module flags (DisableOptimizations,Max64UAVs, andUAVsAtEveryStageflags)