[PIX] Fix mesh shader instrumentation - #8855
Damyan Pepper (damyanp) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes PIX mesh-shader instrumentation so amplification and mesh shaders can share an expanded payload layout.
Changes:
- Reports and consumes expanded payload size and offset.
- Adds payload limits, alignment fixes, signed 16-bit handling, and declaration cleanup.
- Adds regression and validation coverage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
utils/hct/hctdb.py |
Registers payload-layout pass options. |
tools/clang/unittests/HLSL/PixTest.cpp |
Adds PIX instrumentation regression tests. |
tools/clang/test/HLSLFileCheck/pix/SynthesizedPayloadTypeShapes.hlsl |
Tests synthesized payload layouts. |
tools/clang/test/HLSLFileCheck/pix/SynthesizedPayloadTypeRejectsUnusableLayout.hlsl |
Tests invalid-layout rejection. |
tools/clang/test/HLSLFileCheck/pix/MeshShaderWithoutEmitIndicesLeavesNoDeadDeclaration.hlsl |
Verifies declaration cleanup. |
tools/clang/test/HLSLFileCheck/pix/MeshPayloadExpansionAgreesWithAmplificationShader.hlsl |
Verifies AS/MS layout agreement. |
tools/clang/test/HLSLFileCheck/pix/MeshOutputSignedInt16IsSignExtended.hlsl |
Verifies signed 16-bit extension. |
tools/clang/test/HLSLFileCheck/pix/AddThreadIdWhenMSPayloadIsUnused.hlsl |
Updates unused-payload expectations. |
lib/DxilPIXPasses/DxilPIXMeshShaderOutputInstrumentation.cpp |
Reconstructs payload layouts and fixes output instrumentation. |
lib/DxilPIXPasses/DxilPIXAddTidToAmplificationShaderPayload.cpp |
Reports layout and enforces payload constraints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } else { | ||
| expanded = ExpandStructType(Ctx, OriginalPayloadStructType); | ||
| AppendedFieldsElementIndex = | ||
| OriginalPayloadStructType->getStructNumElements(); |
| auto const *expandedLayout = M.getDataLayout().getStructLayout( | ||
| cast<StructType>(expanded.ExpandedPayloadStructType)); |
| uint32_t ExpandedSizeInBytes, uint32_t AppendedFieldsOffsetInBytes, | ||
| unsigned *AppendedFieldsElementIndex) { | ||
| ExpandedStruct ret = {}; | ||
| auto *OriginalStructType = dyn_cast<StructType>(OriginalPayloadStructType); |
| auto as = Compile(m_dllSupport, hlsl, L"as_6_6", {}, L"ASMain"); | ||
| auto asOutput = RunDxilPIXAddTidToAmplificationShaderPayloadPass(as); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The mesh pass still applies a locally derived payload layout when no amplification layout is forwarded.
Review effort: Balanced
Findings: 1
Open (5)
Avoid fallback mesh expansion when forwarded payload layout is missing · New When no expanded payload size is forwarded, this branch still derives and installs a local layout.… Use explicit CComPtr<IDxcBlob> declarations Use explicit StructType pointer declarations Use explicit StructLayout pointer declaration
| } else { | ||
| expanded = ExpandStructType(Ctx, OriginalPayloadStructType); |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Mesh payloads are still locally expanded when the coordinator omits the amplification-shader layout, contrary to the stated contract.
Review effort: Balanced
Findings: 1
Open (5)
Avoid fallback mesh expansion when forwarded payload layout is missing When no expanded payload size is forwarded, this branch still derives and installs a local layout.… Use explicit CComPtr<IDxcBlob> declarations Use explicit StructType pointer declarations Use explicit StructLayout pointer declaration
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The fallback can still create mismatched layouts, and expansion can exceed the combined mesh input/output limit.
Review effort: Balanced
Findings: 2
Open (6)
Account for mesh output footprint in payload expansion limit · New Avoid fallback mesh expansion when forwarded payload layout is missing When no expanded payload size is forwarded, this branch still derives and installs a local layout.… Use explicit CComPtr<IDxcBlob> declarations Use explicit StructType pointer declarations Use explicit StructLayout pointer declaration
| ExpandedSizeInBytes % sizeof(uint32_t) == 0 && | ||
| ExpandedSizeInBytes >= | ||
| AppendedFieldsOffsetInBytes + AppendedFieldsSizeInBytes && | ||
| ExpandedSizeInBytes <= DXIL::kMaxMSASPayloadBytes; |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Missing coordinator layout options still trigger the incompatible local payload-layout fallback.
Review effort: Balanced
Findings: 2
Open (7)
Account for mesh output footprint in payload expansion limit Avoid fallback mesh expansion when forwarded payload layout is missing When no expanded payload size is forwarded, this branch still derives and installs a local layout.… Use explicit LLVM Type pointer types instead of auto · New Use explicit CComPtr<IDxcBlob> declarations Use explicit StructType pointer declarations Use explicit StructLayout pointer declaration
| constexpr uint32_t AppendedFieldsSizeInBytes = 3 * sizeof(uint32_t); | ||
| const DataLayout &DL = M.getDataLayout(); | ||
| const StructLayout *OriginalLayout = DL.getStructLayout(OriginalStructType); | ||
| auto *Int32Type = Type::getInt32Ty(Ctx); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The mesh pass still applies a local fallback layout when the coordinator provides no amplification layout, contradicting the intended safety behavior.
Review effort: Balanced
Findings: 2
Open (8)
Account for mesh output footprint in payload expansion limit Avoid fallback mesh expansion when forwarded payload layout is missing When no expanded payload size is forwarded, this branch still derives and installs a local layout.… Use explicit const std::string reference type · New Use explicit LLVM Type pointer types instead of auto Use explicit CComPtr<IDxcBlob> declarations Use explicit StructType pointer declarations Use explicit StructLayout pointer declaration
| auto asOutput = RunDxilPIXAddTidToAmplificationShaderPayloadPass(as); | ||
| auto lines = Tokenize(Disassemble(asOutput), "\n"); | ||
| bool foundAlloca = false; | ||
| for (auto const &line : lines) { |
8533f48 to
2157109
Compare
Reconstruct the mesh shader payload from the amplification shader's reported expanded layout instead of a locally derived one, instrument mesh shaders whose payload is unread, sign-extend signed 16-bit outputs, remove unused EmitIndices declarations, and fix the reservation assertion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93
2157109 to
f5cd38e
Compare



PIX instruments an amplification shader by expanding its payload to carry a group identifier. The mesh shader must agree on the resulting layout. When the two cannot be reconciled, the pass applies a locally derived layout that is known to disagree. The instrumented pair then disagrees about where each payload field lives.
A mesh shader whose payload is never read is skipped, so its output cannot be instrumented at all. A signed 16-bit output is zero-extended instead of sign-extended. Unused EmitIndices declarations stay in the module, and the validator refuses it. The reservation assertion checks a value that is already zero, so it does not catch a caller that asks for enough space to overwrite the offset counter.
The amplification shader reports the size and offset of the expanded payload through the expanded-payload-size and expanded-payload-offset options. The mesh shader reconstructs its payload type from that report. When reconstruction fails, the pass reports the failure and leaves the mesh payload unchanged. A mismatched pair produces records PIX cannot interpret, so there is no fallback layout.
The pass does not expand a payload that is already near the size limit.
If the PIX coordinator does not forward the layout of the amplification shader, the mesh shader leaves its payload unchanged. That is an integration item for PIX.
Assisted-by: Copilot
Stack created with GitHub Stacks CLI • Give Feedback 💬