microsoft / microsoft/DirectXShaderCompiler

Validation: Mesh/Amplification DXIL op checks insufficient and inefficient

Open
#7,151 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug validation
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.