microsoft / microsoft/DirectXShaderCompiler
Validation: Mesh/Amplification DXIL op checks insufficient and inefficient
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 3.7k
- Forks
- 900
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 44
Description
The following checks are intended to diagnose when multiple calls are present to a DXIL op that allows only one call from the entry function (SetMeshOutputCounts, GetMeshPayload, DispatchMesh):
https://github.com/microsoft/DirectXShaderCompiler/blob/d5951b74d76351c17626bf90daf6684ed8598d3d/lib/DxilValidation/DxilValidation.cpp#L2729-L2759
as well as capture that call for later deeper validation after instruction iteration in ValidateMsIntrinsics and ValidateAsIntrinsics:
https://github.com/microsoft/DirectXShaderCompiler/blob/d5951b74d76351c17626bf90daf6684ed8598d3d/lib/DxilValidation/DxilValidation.cpp#L3009-L3011
ValidateMsIntrinsics creates a DominatorTree and iterates all instructions again in order to ensure that SetMeshOutputCounts is called before stores to verts/prims/indices.
https://github.com/microsoft/DirectXShaderCompiler/blob/d5951b74d76351c17626bf90daf6684ed8598d3d/lib/DxilValidation/DxilValidation.cpp#L2236-L2242
ValidateAsIntrinsics creates a PostDominatorTree to verify that the DispatchMesh call post-dominates the entry block.
https://github.com/microsoft/DirectXShaderCompiler/blob/d5951b74d76351c17626bf90daf6684ed8598d3d/lib/DxilValidation/DxilValidation.cpp#L2352
First, a DominatorTree or PostDominatorTree should be created once and cached for any given function, instead of locally (re)computed whenever needed. Other functions creating DominatorTree: ValidateFlowControl, and PostDominatorTree: ValidateTGSMRaceCondition. These should be created and cached by the ValidationContext object instead.
Second, it seems excessive to iterate every instruction in the function again in ValidateMsIntrinsics just to find the call we just found while iterating every instruction. The point is to look for any writes to outputs so the dominance check will catch writes before the SetMeshOutputCounts call. However, there should be a better way, such as by collecting SetMeshOutputCounts calls by calling function (entry) in advance (checking uniqueness at the same time), then with the DominatorTree cache available, just check the store vert/prim/indices DXIL op calls for dominance by the associated SetMeshOutputCounts previously collected for the function (kept by ValidationContext). If it doesn't dominate (or is missing), that's an error case.
Third, this approach is currently insufficient, since it's possible to have a [noinline] user function that calls one of these DXIL ops. This call must be attributed to the entry function that leads to the call, or disallowed entirely. Disallowing the call in [noinline] user functions is the easiest.
If we wanted to allow these calls, there would be some work to do, and it should be noted that for library targets, validation currently treats [noinline] functions as if they are export functions, which means that these particular calls are currently disallowed in [noinline] functions, even though they are not really exported functions.
Shader that passes validation, but should fail: https://godbolt.org/z/8MW9rdhqE
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in lib/DxilValidation/DxilValidation.cpp by reading ValidateMsIntrinsics, ValidateAsIntrinsics, ValidateFlowControl, ValidateTGSMRaceCondition, and ValidationContext, then reproduce the linked shader that currently passes validation. The work is complete when dominance information is cached, redundant instruction iteration is removed, and calls from [noinline] user functions are handled so invalid DXIL is rejected.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100