[PIX] Inline helper functions before shader debugging - #8853
Damyan Pepper (damyanp) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates PIX instrumentation to inline helper functions, support older shader models, and avoid unnecessary debug UAVs.
Changes:
- Inlines non-entry helpers before PIX prepasses and reports survivors.
- Instruments hull patch-constant functions.
- Uses
BufferStorebefore shader model 6.2 and adds regression coverage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tools/clang/unittests/HLSL/PixTest.cpp |
Adds PIX regression tests and helpers. |
tools/clang/test/HLSLFileCheck/pix/NonUniformResourceIndexInHelperFunction.hlsl |
Tests resource indexing after helper inlining. |
tools/clang/test/HLSLFileCheck/pix/DebugStoreOpcodeByShaderModel.hlsl |
Tests store opcode selection. |
tools/clang/test/HLSLFileCheck/pix/DebugNoInlineHelperFunction.hlsl |
Tests debug information after inlining. |
tools/clang/test/HLSLFileCheck/pix/DebugHullPatchConstantFunction.hlsl |
Tests hull patch-constant instrumentation. |
tools/clang/test/HLSLFileCheck/pix/DebugBreakInstrumentationInHelperFunction.hlsl |
Tests debug-break helper inlining. |
lib/DxilPIXPasses/PixPassHelpers.h |
Declares the inlining helper. |
lib/DxilPIXPasses/PixPassHelpers.cpp |
Implements helper inlining and survivor collection. |
lib/DxilPIXPasses/DxilDebugInstrumentation.cpp |
Updates instrumentation targets and store operations. |
lib/DxilPIXPasses/DxilDbgValueToDbgDeclare.cpp |
Runs inlining before shadow-storage generation. |
lib/DxilPIXPasses/DxilAnnotateWithVirtualRegister.cpp |
Runs inlining before annotation and reports survivors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (M.HasDxilModule() || | ||
| M.getNamedMetadata(hlsl::DxilMDHelper::kDxilVersionMDName) != nullptr) { | ||
| PIXPassHelpers::InlineNonEntryFunctions(M.GetOrCreateDxilModule()); | ||
| } | ||
|
|
||
| auto GlobalEmbeddedArrayStorage = GatherGlobalEmbeddedArrayStorage(M); | ||
|
|
||
| bool Changed = false; |
| // llvm::InlineFunction is the mechanical inliner and ignores inlining | ||
| // attributes. Clear the attribute so the module carries no claim that | ||
| // contradicts its own shape. | ||
| function->removeFnAttr(llvm::Attribute::NoInline); |
| llvm::SmallVector<llvm::Function *, 4> UninlinedFunctions; | ||
| PIXPassHelpers::InlineNonEntryFunctions(M.GetOrCreateDxilModule(), | ||
| &UninlinedFunctions); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Inlining can produce invalid function attributes and two passes fail to report inlining-only module changes.
Review effort: Balanced
Findings: 1
Open (4)
Preserve noinline on surviving optnone helpers · New The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded…
| // llvm::InlineFunction is the mechanical inliner and ignores inlining | ||
| // attributes. Clear the attribute so the module carries no claim that | ||
| // contradicts its own shape. | ||
| function->removeFnAttr(llvm::Attribute::NoInline); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Recursive IR can cause unbounded inlining, and two passes incorrectly discard their inlining change status.
Review effort: Balanced
Findings: 2
Open (7)
Handle recursive call graphs to prevent nonterminating inlining · New Preserve noinline on surviving optnone helpers The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded… Add a release note for the PIX instrumentation fix · New Use explicit hlsl::DxilResource pointer type · New
| // HLSL has no recursion, so the call graph is acyclic and inlining leaf-ward | ||
| // terminates. A fixed-point loop also reaches a helper that loses its last | ||
| // caller only once another helper is inlined away. | ||
| bool inlinedACallThisRound = true; | ||
| while (inlinedACallThisRound) { | ||
| inlinedACallThisRound = false; |
| // RawBufferStore is only legal from shader model 6.2 onwards. PIX also | ||
| // instruments 6.0 and 6.1 shaders, so fall back to BufferStore (legal from | ||
| // 6.0) on those. The two differ only in the trailing alignment operand. | ||
| const bool SupportsRawBufferStore = BC.DM.GetShaderModel()->IsSM62Plus(); |
| return false; | ||
| } | ||
|
|
||
| auto *uav = PIXPassHelpers::CreateGlobalUAVResource(DM, HLSLBindId, "PIXUAV"); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple passes mutate the module without consistently reporting the change, risking stale cached analyses.
Review effort: Balanced
Findings: 2
Open (13)
Handle recursive call graphs to prevent nonterminating inlining Preserve noinline on surviving optnone helpers The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded… Use the explicit loop element type · New Use the explicit cast target type · New Use explicit types instead of auto · New Use explicit result types instead of auto · New Spell out CComPtr<IDxcBlob> instead of auto · New Use explicit result types instead of auto · New Use explicit hlsl::DxilResource pointer type Add a release note for the PIX instrumentation fix
|
|
||
| auto *uav = PIXPassHelpers::CreateGlobalUAVResource(DM, HLSLBindId, "PIXUAV"); | ||
| bool modified = false; | ||
| for (auto *function : functionsToInstrument) { |
| // Collect the call sites first, because inlining rewrites the use list. | ||
| llvm::SmallVector<llvm::CallInst *, 8> callSites; | ||
| for (llvm::User *user : function->users()) { | ||
| if (auto *call = llvm::dyn_cast<llvm::CallInst>(user)) { |
| auto compiled = Compile(m_dllSupport, source, L"lib_6_6", {L"-Od"}); | ||
| CComPtr<IDxcBlob> dxil = FindModule(DFCC_ShaderDebugInfoDXIL, compiled); | ||
| auto output = RunDebugPass(dxil); |
| auto compiled = Compile(m_dllSupport, kDebugStoreOpcodeComputeShader, | ||
| L"cs_6_0", {L"-Od"}); | ||
| auto output = RunDebugPass(compiled); |
| auto compiled = | ||
| Compile(m_dllSupport, kHullShaderWithHelper, L"hs_6_2", {L"-Od"}); |
| auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); | ||
| std::string outputText; | ||
| auto output = | ||
| RunDxilNonUniformResourceIndexInstrumentation(compiled, outputText); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several mutation paths incorrectly report no module modification, and the user-visible fixes need release-note coverage.
Review effort: Balanced
Findings: 2
Open (14)
Handle recursive call graphs to prevent nonterminating inlining Preserve noinline on surviving optnone helpers The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded… Add PIX debugging fixes to the release notes · New Use explicit result types instead of auto Spell out CComPtr<IDxcBlob> instead of auto Use explicit result types instead of auto Use explicit types instead of auto Use the explicit cast target type Use the explicit loop element type Use explicit hlsl::DxilResource pointer type Add a release note for the PIX instrumentation fix
| // Inlines each function that the runtime does not invoke into its callers, and | ||
| // erases the inlined-away body. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Several passes fail to report inlining mutations accurately, survivor attributes are altered incorrectly, and release-note coverage is missing.
Review effort: Balanced
Findings: 2
Open (14)
Handle recursive call graphs to prevent nonterminating inlining Preserve noinline on surviving optnone helpers The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded… Add PIX debugging fixes to the release notes Use explicit result types instead of auto Spell out CComPtr<IDxcBlob> instead of auto Use explicit result types instead of auto Use explicit types instead of auto Use the explicit cast target type Use the explicit loop element type Use explicit hlsl::DxilResource pointer type Add a release note for the PIX instrumentation fix
Previously missed (1)
In code that hasn't changed since last review

Spell out explicit types instead of using type deduction

lib/DxilPIXPasses/DxilDebugInstrumentation.cpp:1356
These three types are short and unambiguous, so type deduction conflicts with the repository's explicit “almost never auto” convention. Spell out the shader-model pointer, shader kind, and binding-index types here.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Several new inlining paths report no modification despite changing the module, and release-note coverage remains unresolved.
Review effort: Balanced
Findings: 2
Open (14)
Handle recursive call graphs to prevent nonterminating inlining Preserve noinline on surviving optnone helpers The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded… Add PIX debugging fixes to the release notes Use explicit result types instead of auto Spell out CComPtr<IDxcBlob> instead of auto Use explicit result types instead of auto Use explicit types instead of auto Use the explicit cast target type Use the explicit loop element type Use explicit hlsl::DxilResource pointer type Add a release note for the PIX instrumentation fix
aa5bcad to
c8b1e52
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Modification reporting, survivor reporting, and release-note coverage need correction before approval.
Review effort: Balanced
Findings: 2
Open (15)
Handle recursive call graphs to prevent nonterminating inlining Preserve noinline on surviving optnone helpers Populate surviving functions when no entry function exists · New The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded… Add PIX debugging fixes to the release notes Use explicit result types instead of auto Spell out CComPtr<IDxcBlob> instead of auto Use explicit result types instead of auto Use explicit types instead of auto Use the explicit cast target type Use the explicit loop element type Use explicit hlsl::DxilResource pointer type Add a release note for the PIX instrumentation fix
| if (entryFunction == nullptr) { | ||
| return false; | ||
| } |
Inline non-entry helpers before numbering so each invocation maps to one function, emit only stores legal for the shader model, avoid creating the tools UAV when nothing is instrumented, instrument hull-shader patch-constant functions, and report functions that survive inlining. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93
c8b1e52 to
bded466
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Core pass-reporting bugs and numerous unrelated compiler regressions must be resolved before approval.
Review effort: Balanced
Findings: 3
Open (25)
Reject LinAlg matrices in unsupported storage contexts · New Handle recursive call graphs to prevent nonterminating inlining Preserve noinline on surviving optnone helpers Preserve invocation counting at zero record capacity · New Restore HLSL static_assert keyword support · New Restore default initialization for ExpandTokPastingArg · New Exclude LinAlgMatrix from numeric aggregate types · New Restore LinAlg vector compatibility constraints · New Preserve HLSL 202x const method support · New Require a digit boundary when matching expected counts · New Populate surviving functions when no entry function exists The inliner’s change result is ignored here, while this pass eventually reports only whether a… RemovingNoInlinemutates the IR even when no call is inlined and the body survives (for example,…InlineNonEntryFunctionscan rewrite calls and delete helper bodies, but its result is discarded… Restore Version 1.9.2609 release heading · New Use explicit llvm::Value pointer declarations · New Add PIX debugging fixes to the release notes Use explicit result types instead of auto Spell out CComPtr<IDxcBlob> instead of auto Use explicit result types instead of auto
And 5 more that still need to be addressed.
| HLSLExternalSource *Source = HLSLExternalSource::FromSema(&S); | ||
|
|
||
| const Type *CanonTy = Ty.getCanonicalType().getTypePtr(); | ||
| if (CanonTy->isAttributedLinAlgMatrixType() || | ||
| CanonTy->isLinAlgMatrixType()) { | ||
| Empty = false; | ||
| if (!CheckObjects || AllowLinAlgMatrixInContext(ObjDiagContext)) | ||
| return false; | ||
| S.Diag(Loc, diag::err_hlsl_unsupported_object_context) | ||
| << Ty << ObjDiagContextIdx; | ||
| if (FD) | ||
| S.Diag(FD->getLocation(), diag::note_field_declared_here) | ||
| << FD->getType() << FD->getSourceRange(); | ||
| return true; | ||
| } | ||
|
|
||
| ArTypeObjectKind ShapeKind = Source->GetTypeObjectKind(Ty); | ||
| switch (ShapeKind) { |
| // A zero-entry log has no space for records. | ||
| if (m_MaxNumEntriesInLog == 0) { | ||
| return false; | ||
| } |
| CXX11_KEYWORD(noexcept , 0) | ||
| CXX11_KEYWORD(nullptr , 0) | ||
| CXX11_KEYWORD(static_assert , KEYHLSL2026) // HLSL Change - 2026 adds static_assert | ||
| CXX11_KEYWORD(static_assert , 0) |
| PreprocessorOptions() | ||
| : UsePredefines(true), DetailedRecord(false), | ||
| // HLSL Change Begin - ignore line directives. | ||
| IgnoreLineDirectives(false), ExpandTokPastingArg(false), | ||
| // HLSL Change End | ||
| IgnoreLineDirectives(false), // HLSL Change - ignore line directives. | ||
| DisablePCHValidation(false), AllowPCHWithCompilerErrors(false), |
| // Chars can only appear as part of strings, which we don't consider numeric. | ||
| // LinAlg matrix handles are opaque objects, not numeric data. | ||
| const BuiltinType *BuiltinTy = dyn_cast<BuiltinType>(Ty); | ||
| return BuiltinTy != nullptr && | ||
| BuiltinTy->getKind() != BuiltinType::Kind::Char_S && | ||
| BuiltinTy->getKind() != BuiltinType::Kind::LinAlgMatrix; | ||
| BuiltinTy->getKind() != BuiltinType::Kind::Char_S; |
| template <ComponentEnum DT, typename T, int N> | ||
| InterpretedVector<T, N, DT> MakeInterpretedVector(vector<T, N> Vec) { | ||
| InterpretedVector<T, N, DT> IV = {Vec}; | ||
| return IV; |
| // HLSL Change Starts | ||
| if (getLangOpts().HLSL) { | ||
| Diag(DS.getSourceRange().getEnd(), | ||
| diag::err_hlsl_unsupported_construct) | ||
| << "qualifiers"; | ||
| } |
| const std::string expectedSuffix = ", " + std::to_string(expectedEntryCount); | ||
| for (auto const &line : lines) { | ||
| if (line.find("icmp ult i32 %EntryIndexResult") != std::string::npos && | ||
| line.find(expectedSuffix) != std::string::npos) { | ||
| return true; | ||
| } |
| - Fixed an optimizer crash when scalarizing an out-of-bounds vector access | ||
| [#8940](https://github.com/microsoft/DirectXShaderCompiler/issues/8940). | ||
|
|
||
| ### Upcoming Preview Release | ||
|
|
||
| These changes apply to experimental preview shader models only and will not be | ||
| part of the next non-preview release. | ||
|
|
||
| #### Experimental Shader Model 6.11 | ||
|
|
||
| - Added experimental Shader Model 6.11 target profiles. | ||
|
|
||
| ### Version 1.9.2609 | ||
|
|
||
| #### Bug Fixes | ||
|
|
||
| - Fixed derivative operations being moved into divergent control flow, which |
| auto EntryOffset = Builder.CreateMul( | ||
| EntryIndex, HlslOP->GetU32Const(numBytesPerEntry), "EntryOffset"); | ||
| Value *EntryOffsetPlus16 = Builder.CreateAdd( | ||
| auto EntryOffsetPlus16 = Builder.CreateAdd( | ||
| EntryOffset, HlslOP->GetU32Const(16), "EntryOffsetPlus16"); | ||
| Value *EntryOffsetPlus32 = Builder.CreateAdd( | ||
| auto EntryOffsetPlus32 = Builder.CreateAdd( | ||
| EntryOffset, HlslOP->GetU32Const(32), "EntryOffsetPlus32"); | ||
| Value *EntryOffsetPlus48 = Builder.CreateAdd( | ||
| auto EntryOffsetPlus48 = Builder.CreateAdd( |


PIX maps one shader invocation to one record stream in the debug UAV, and one stream to exactly one function. A [noinline] helper instrumented as its own function looks like a second invocation of a thread that runs once. PIX discards those records, and you cannot step into the helper.
The debug instrumentation always emits RawBufferStore. That operation is legal only from shader model 6.2, so shader models 6.0 and 6.1 get an invalid module.
The pass creates the tools UAV before it knows whether there is anything to instrument. A library that contains only helpers therefore gains a UAV although the pass reports that it changed nothing.
Inlining happens before the pass numbers instructions or creates shadow storage. Every prepass does it, so the debug, non-uniform-resource-index, and debug-break pipelines all see the same module shape. A library module is left alone, because each exported function is its own invocation. The runtime invokes the patch-constant function of a hull shader directly. That function therefore survives inlining, and the pass instruments it. The pass selects an invocation of it by primitive alone, because OutputControlPointID is valid only in the control point phase.
A function that survives inlining is named in the pass report as UninlinedFunction:, so PIX does not offer a range with no records. The pass reports it and continues, instead of stopping the process.
Assisted-by: Copilot
Stack created with GitHub Stacks CLI • Give Feedback 💬