RDAT: Check flags and WaveSize for min SM; fix flag detection and merging #6207

tex3d · 2024-01-25T22:38:57Z

Add ShaderKind::Last_1_8 for shader mask
Add shader model comments before flag groupings in DxilConstants.h and DxilShaderFlags.

Add missing flag checks for min shader model in RDAT function info. Move ShaderCompatInfo computation into DxilModule, propagate callee info.

Move computation of shader model requirements based on flags into DxilModule. Finalize requirements for entry functions in AdjustMinimumShaderModelAndFlags.

Fixes for function level flag tracking:

DerivativesInMeshAndAmpShaders: use flag to track deriv use, then adjust for entry functions.
hasUAVs: based on resource use in function instead of global resources.
WriteableMSAATextures: based on use in function instead of global resources.
- Also catch cases for dynamic res from any use by looking at create/annotate handle, not just the TextureStoreSample op.
RaytracingTier1_1: move module-level detection to CollectShaderFlagsForModule
Marked deriv and quad ops as being supported in node.
Fixed SampleCmpBias to be considered gradient op.
Update RDAT definitions to dump more useful info for testing

Fixes #6218.

github-actions · 2024-01-25T22:39:53Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Adding OptFeatureInfo_UsesDerivatives to track per-function derivative usage, otherwise we can't set DerivativesInMeshAndAmpShaders or min shader target properly. This uses a feature bit in a special optional set of flags defined by the runtime to be ignored if set and unrecognized. Since it's not a real feature requirement but a special flag used in computing feature requirements after accounting for all functions at link time, this is a good place for this special flag.

hasUAVs and hasWriteableMSAATextures were set based on global resource properties, instead of what the function actually used. This change fixes that, additionally detecting cases that would have been missed by catching all handle cases.

This check did not belong in per-function CollectShaderFlags. Moved to module.

OP::GetMinShaderModelAndMask needed to account for new Node shader. Also, the barrier op check was not robust to an invalid operand, hit by validation test.

…rget Feature flags were not taken into account when computing minimum shader targets. Also, once a function is known to be a particular shader, we can translate the UsesDerivatives into the correct min target and potential DerivativesInMeshAndAmpShaders feature flag. This also takes into account min target for WaveSize and WaveSize with Range.

ShaderCompatInfo was used to track per-function feature flag and min target for RDAT. But internal calls were not traversed and the global feature flag collection had to do all the same work as the RDAT writer did redundantly. This change moves this into DxilModule, where it can be computed once and cached for both the CollectShaderFlagsForModule, as well as the RDAT per-function lookup. Along the way, DxilModule& needed to be made non-const for DxilRDATWriter so it could update the compat info, in case it was unwritten.

…eFlags

tex3d · 2024-01-30T03:00:45Z

If you start here, then step through commits with [Next], it's a lot more digestible.

include/dxc/DXIL/DxilConstants.h

include/dxc/DXIL/DxilModule.h

include/dxc/DXIL/DxilConstants.h

lib/DXIL/DxilOperations.cpp

include/dxc/DXIL/DxilConstants.h

include/dxc/DXIL/DxilModule.h

Add tests for several classes of deriv ops in node. This also adds testing for barrier in node, which was a fix in this PR.

github-actions · 2024-02-06T08:25:18Z

✅ With the latest revision this PR passed the Python code formatter.

pow2clk

Not quite done I'm afraid, but I wanted to give what I had.

A lot of this is nits to make it more readable, but there are some substantive comments too. Let me know if any are unclear which they are.

include/dxc/DXIL/DxilConstants.h

include/dxc/DXIL/DxilModule.h

include/dxc/DXIL/DxilShaderFlags.h

lib/DXIL/DxilModule.cpp

One notable change is the elimination of the optimization in ComputeShaderCompatInfo where a function would be skipped if it already had ShaderCompatInfo. This was incorrect, since this is happening inside the same loop that iterates intrinsics, which could add the compat info based on intrinsic usage. That doesn't mean flags have been computed for the function, so I removed this and clear the map at the beginning of ComputeShaderCompatInfo to ensure it's fresh. I also added IR tests, and ones that reorder intrinsics and the called function to ensure function ordering doesn't cause a problem. I made these and the sm66_derivatives.hlsl test they were based on go back to SM 6.6, since there was another detail on that validator version that was different - it didn't set min shader model properly for lib functions. The 1.6 validator is the one included in the latest Windows SDK, so going back this far seems good. I also added the OptFeatureInfo flags to disassembly.

Function-level UsesDerivatives flag is not necessary at global library level, so always clear it in CollectShaderFlagsForModule. Prior code used !(SM->IsCS() || SM->IsPS()), which covered any new shader stages. This flag only matters for VS, HS, DS, and GS. I fixed the code, but left the old logic in for validator version 1.7 and below for compatibility. Add bit index comments for the raw ShaderFlags bitfields to make decoding raw flags easier. Fix metadata for validator version 1.7 in SM 6.7 IL test. Add test for global ShaderFlags checking: - masking of UsesDerivatives - DerivativesInMeshAndAmpShaders from called function - removal of UAVsAtEveryStage for library profile

pow2clk

I think with a long change like this, my comments tend toward complaining about how hard it is to read and review. Sorry about that.

I tried to make suggested changes where possible. Overall, this improves more than I expected to fix this bug. Now that it's done, that's great!

I don't think much needs to change, I just have a couple concerns and many readability suggestions.

lib/DXIL/DxilOperations.cpp

lib/DXIL/DxilShaderFlags.cpp

lib/HLSL/DxilContainerReflection.cpp

tools/clang/unittests/HLSLTestLib/FileCheckerTest.cpp

lib/DXIL/DxilShaderFlags.cpp

lib/DxilContainer/DxilContainerAssembler.cpp

Co-authored-by: Greg Roth <[email protected]>

lib/DXIL/DxilModule.cpp

pow2clk

Love the pretty printing! Thanks for all the responses!

tools/clang/unittests/HLSLTestLib/FileCheckerTest.cpp

lib/HLSL/DxilContainerReflection.cpp

…ging (microsoft#6207) Add ShaderKind::Last_1_8 for shader mask Add shader model comments before flag groupings in DxilConstants.h and DxilShaderFlags. Add missing flag checks for min shader model in RDAT function info. Move ShaderCompatInfo computation into DxilModule, propagate callee info. Move computation of shader model requirements based on flags into DxilModule. Finalize requirements for entry functions in AdjustMinimumShaderModelAndFlags. Fixes for function level flag tracking: - DerivativesInMeshAndAmpShaders: use flag to track deriv use, then adjust for entry functions. - hasUAVs: based on resource use in function instead of global resources. - WriteableMSAATextures: based on use in function instead of global resources. - Also catch cases for dynamic res from any use by looking at create/annotate handle, not just the TextureStoreSample op. - RaytracingTier1_1: move module-level detection to CollectShaderFlagsForModule - Marked deriv and quad ops as being supported in node. - Fixed SampleCmpBias to be considered gradient op. - Update RDAT definitions to dump more useful info for testing Fixes microsoft#6218. (cherry picked from commit 9c518db)

…ging (#6207) (#6273) Add ShaderKind::Last_1_8 for shader mask Add shader model comments before flag groupings in DxilConstants.h and DxilShaderFlags. Add missing flag checks for min shader model in RDAT function info. Move ShaderCompatInfo computation into DxilModule, propagate callee info. Move computation of shader model requirements based on flags into DxilModule. Finalize requirements for entry functions in AdjustMinimumShaderModelAndFlags. Fixes for function level flag tracking: - DerivativesInMeshAndAmpShaders: use flag to track deriv use, then adjust for entry functions. - hasUAVs: based on resource use in function instead of global resources. - WriteableMSAATextures: based on use in function instead of global resources. - Also catch cases for dynamic res from any use by looking at create/annotate handle, not just the TextureStoreSample op. - RaytracingTier1_1: move module-level detection to CollectShaderFlagsForModule - Marked deriv and quad ops as being supported in node. - Fixed SampleCmpBias to be considered gradient op. - Update RDAT definitions to dump more useful info for testing Fixes #6218. (cherry picked from commit 9c518db)

tex3d force-pushed the rdat-minsm-check-flags branch from cbacb36 to 79f021c Compare January 26, 2024 20:57

tex3d added 12 commits January 29, 2024 17:14

Add DXIL::ShaderKind::Last_1_8

7f7f2af

Add shader model comments for feature flags; add missing const

aa77c81

NFC: Clarified name and comments around canSetHasLodClamp

deb6c82

Move global DXR 1.1 check to CollectShaderFlagsForModule

429f897

This check did not belong in per-function CollectShaderFlags. Moved to module.

Fix OP::GetMinShaderModelAndMask for Node and make robust

422971f

OP::GetMinShaderModelAndMask needed to account for new Node shader. Also, the barrier op check was not robust to an invalid operand, hit by validation test.

Mask off OptFeatureInfo_UsesDerivatives for reflection RequiredFeatur…

36ef98b

…eFlags

NFC: Fixed some garbled comments in DxilContainerReflection

d44bd2c

Update and add tests.

e99711e

tex3d force-pushed the rdat-minsm-check-flags branch from 7d883a9 to e99711e Compare January 30, 2024 02:25

NFC: Formatting

27b663b

tex3d marked this pull request as ready for review January 30, 2024 02:38

NFC: Formatting

9d3b6ba

tex3d requested review from pow2clk and python3kgae January 30, 2024 02:55

python3kgae reviewed Jan 30, 2024

View reviewed changes

include/dxc/DXIL/DxilConstants.h Outdated Show resolved Hide resolved

python3kgae reviewed Jan 30, 2024

View reviewed changes

include/dxc/DXIL/DxilModule.h Show resolved Hide resolved

coopp reviewed Jan 30, 2024

View reviewed changes

include/dxc/DXIL/DxilConstants.h Show resolved Hide resolved

coopp reviewed Jan 30, 2024

View reviewed changes

lib/DXIL/DxilOperations.cpp Show resolved Hide resolved

coopp reviewed Jan 30, 2024

View reviewed changes

include/dxc/DXIL/DxilConstants.h Outdated Show resolved Hide resolved

coopp reviewed Jan 30, 2024

View reviewed changes

include/dxc/DXIL/DxilModule.h Show resolved Hide resolved

tex3d added 3 commits January 31, 2024 15:12

Rename MaxOfShaderModels to UpdateToMaxOfVersions

bfe9944

Add comment for shader stage mask in ShaderCompatInfo

37993eb

Fix stage flags for CS/MS/AS when using derivatives in 1.8

31a7ba6

coopp approved these changes Feb 1, 2024

View reviewed changes

tex3d added 2 commits February 5, 2024 13:57

node supports deriv and quad ops; SampleCmpBias as gradient

2c97e89

Add tests for several classes of deriv ops in node. This also adds testing for barrier in node, which was a fix in this PR.

Make RuntimeDataFunctionInfo_Reader::GetFeatureFlags const

1f1649f

tex3d force-pushed the rdat-minsm-check-flags branch from 331747a to 1f1649f Compare February 6, 2024 18:47

pow2clk reviewed Feb 7, 2024

View reviewed changes

tex3d requested a review from pow2clk February 8, 2024 03:18

pow2clk reviewed Feb 8, 2024

View reviewed changes

tex3d and others added 6 commits February 8, 2024 15:39

Fix test metadata that got broken last-minute and remove unintended test

d3b00fb

Apply suggestions from code review

da61493

Co-authored-by: Greg Roth <[email protected]>

Address feedback, fix subtle 1.7 compat issue with WriteableMSAA

2770926

Use RDAT_FLAGS and add RDAT_VALUE_HEX to simplify checks

bf005d8

chore: autopublish 2024-02-09T08:02:54Z

84cf5b1

Move AdjustMinimumShaderModelAndFlags to DxilModule.cpp as static

5dcdfb9

python3kgae reviewed Feb 9, 2024

View reviewed changes

lib/DXIL/DxilModule.cpp Show resolved Hide resolved

pow2clk approved these changes Feb 9, 2024

View reviewed changes

python3kgae reviewed Feb 9, 2024

View reviewed changes

tools/clang/unittests/HLSLTestLib/FileCheckerTest.cpp Show resolved Hide resolved

python3kgae reviewed Feb 9, 2024

View reviewed changes

lib/HLSL/DxilContainerReflection.cpp Show resolved Hide resolved

tex3d mentioned this pull request Feb 10, 2024

Test limitation: %D3DReflect only loads from prior stage stdout, preventing IR-based test #6271

Closed

tex3d merged commit 9c518db into microsoft:main Feb 10, 2024
13 checks passed

tex3d deleted the rdat-minsm-check-flags branch February 10, 2024 08:12

pow2clk mentioned this pull request Feb 16, 2024

Don't set RaytracingTier1_1 based on subobjects; enable flag validation #6320

Merged

tex3d mentioned this pull request Feb 16, 2024

RaytracingTier1_1 flag should not be set in module based on subobject #6321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RDAT: Check flags and WaveSize for min SM; fix flag detection and merging #6207

RDAT: Check flags and WaveSize for min SM; fix flag detection and merging #6207

tex3d commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

tex3d commented Jan 30, 2024

github-actions bot commented Feb 6, 2024 •

edited

Loading

pow2clk left a comment

pow2clk left a comment

pow2clk left a comment

RDAT: Check flags and WaveSize for min SM; fix flag detection and merging #6207

RDAT: Check flags and WaveSize for min SM; fix flag detection and merging #6207

Conversation

tex3d commented Jan 25, 2024 • edited Loading

github-actions bot commented Jan 25, 2024 • edited Loading

tex3d commented Jan 30, 2024

github-actions bot commented Feb 6, 2024 • edited Loading

pow2clk left a comment

Choose a reason for hiding this comment

pow2clk left a comment

Choose a reason for hiding this comment

pow2clk left a comment

Choose a reason for hiding this comment

tex3d commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024 •

edited

Loading