[PIX] Attribute library accesses to reaching entry points - #8849
Damyan Pepper (damyanp) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Attributes PIX resource-access records in library helpers to their reaching shader entry-point kind and prevents instruction ordinals from corrupting adjacent record fields.
Changes:
- Resolves shader kinds through the library call graph.
- Masks instruction ordinals to 24 bits.
- Adds library-helper and bindless-access tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/DxilPIXPasses/DxilShaderAccessTracking.cpp |
Adds shader-kind propagation and safe record encoding. |
tools/clang/test/HLSLFileCheck/pix/AccessTrackingLibHelperShaderKind.hlsl |
Verifies ray-generation encoding in a helper. |
tools/clang/unittests/HLSL/PixTest.cpp |
Adds instrumentation and validation tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| auto emplaced = | ||
| functionToShaderKind.emplace(reached, entryPointShaderKind); | ||
| if (!emplaced.second && emplaced.first->second != entryPointShaderKind) { | ||
| emplaced.first->second = ambiguousShaderKind; | ||
| } |
| uint32_t value = | ||
| static_cast<uint32_t>(strtoul(line.c_str() + position, &end, 10)); |
| constexpr uint32_t InstructionOrdinalMask = 0x00FF'FFFF; | ||
| uint32_t EncodedInstructionNumber = | ||
| (InstructionNumber & InstructionOrdinalMask) | |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Core overflow and multi-stage ambiguity behaviors need direct regression coverage.
Review effort: Balanced
Findings: 2
Open (4)
The 24-bit truncation has no boundary regression test: both new tests use ordinary… No test exercises this ambiguity path. Please add a library where differently typed entry points… Use explicit ConstantInt pointer type · New This introducesstrtoulwithout directly including its declaring standard header. Add<cstdlib>…
| auto *EncodedShaderKindConstant = cast<ConstantInt>( | ||
| m_FunctionToEncodedAccess.at(Builder.GetInsertBlock()->getParent()) | ||
| .at(ResourceAccessStyle::None)); |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The call-graph traversal has scalability concerns and key boundary and ambiguity behavior lacks regression coverage.
Review effort: Balanced
Findings: 3
Open (7)
Avoid rescanning shared helper subtrees per shader export · New The 24-bit truncation has no boundary regression test: both new tests use ordinary… No test exercises this ambiguity path. Please add a library where differently typed entry points… Spell out CallInst pointer type · New Spell out string loop element type · New Use explicit ConstantInt pointer type This introducesstrtoulwithout directly including its declaring standard header. Add<cstdlib>…
| for (llvm::Function *entryPoint : entryPoints) { | ||
| if (entryPoint == nullptr || entryPoint->isDeclaration()) { | ||
| continue; | ||
| } | ||
|
|
||
| const DXIL::ShaderKind entryPointShaderKind = | ||
| PIXPassHelpers::GetFunctionShaderKind(DM, entryPoint); | ||
|
|
||
| std::vector<llvm::Function *> pending{entryPoint}; | ||
| std::set<llvm::Function *> visited; | ||
| while (!pending.empty()) { |
|
|
||
| for (llvm::BasicBlock &block : reached->getBasicBlockList()) { | ||
| for (llvm::Instruction &instruction : block.getInstList()) { | ||
| auto *call = llvm::dyn_cast<llvm::CallInst>(&instruction); |
| static bool | ||
| HasBufferStoreValueMatchingMask(std::vector<std::string> const &lines, | ||
| uint32_t mask, uint32_t maskedValue) { | ||
| for (auto const &line : lines) { |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The multi-stage ambiguity path needs regression coverage, and several declarations violate the explicit-type convention.
Review effort: Balanced
Findings: 3
Open (8)
Avoid rescanning shared helper subtrees per shader export The 24-bit truncation has no boundary regression test: both new tests use ordinary… No test exercises this ambiguity path. Please add a library where differently typed entry points… Use explicit function-pointer vector return type · New Spell out string loop element type Spell out CallInst pointer type Use explicit ConstantInt pointer type This introducesstrtoulwithout directly including its declaring standard header. Add<cstdlib>…
| auto instrumentableFunctions = | ||
| PIXPassHelpers::GetAllInstrumentableFunctions(DM); |
9370fe9 to
7acdd2b
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Library hull patch-constant functions are omitted as traversal roots and remain incorrectly attributed as Library.
Review effort: Balanced
Findings: 4
Open (10)
Include hull patch-constant functions in exported function traversal · New Avoid rescanning shared helper subtrees per shader export The 24-bit truncation has no boundary regression test: both new tests use ordinary… No test exercises this ambiguity path. Please add a library where differently typed entry points… Use the short explicit result type · New Use explicit function-pointer vector return type Spell out string loop element type Spell out CallInst pointer type Use explicit ConstantInt pointer type This introducesstrtoulwithout directly including its declaring standard header. Add<cstdlib>…
| std::map<llvm::Function *, DXIL::ShaderKind> functionToShaderKind; | ||
|
|
||
| const DXIL::ShaderKind ambiguousShaderKind = DM.GetShaderModel()->GetKind(); | ||
| auto entryPoints = DM.GetExportedFunctions(); |
| auto *EncodedShaderKindConstant = cast<ConstantInt>( | ||
| m_FunctionToEncodedAccess.at(Builder.GetInsertBlock()->getParent()) | ||
| .at(ResourceAccessStyle::None)); |
Attribute accesses in library helper functions to the pipeline stage of the entry points that reach them instead of the Library kind, and keep instruction numbers from overflowing the 24-bit record field. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93
7acdd2b to
379abfc
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Library hull patch-constant functions remain incorrectly attributed to the Library shader kind.
Review effort: Balanced
Findings: 4
Open (12)
Include hull patch-constant functions in exported function traversal Avoid rescanning shared helper subtrees per shader export The 24-bit truncation has no boundary regression test: both new tests use ordinary… No test exercises this ambiguity path. Please add a library where differently typed entry points… Use explicit map type instead of auto · New Use explicit types instead of auto in test helpers · New Use the short explicit result type Use explicit function-pointer vector return type Spell out string loop element type Spell out CallInst pointer type Use explicit ConstantInt pointer type This introducesstrtoulwithout directly including its declaring standard header. Add<cstdlib>…
| auto instrumentableFunctions = | ||
| PIXPassHelpers::GetAllInstrumentableFunctions(DM); | ||
|
|
||
| auto functionToShaderKind = ResolveShaderKindByReachingEntryPoint(DM); |
| auto compiled = Compile(m_dllSupport, hlsl, L"lib_6_6", {L"-Od"}); | ||
| auto output = RunShaderAccessTrackingPass(compiled, L".0;0;0."); | ||
| auto lines = Split(Disassemble(output.blob), '\n'); |


The pass either skips a helper function in a library or gives its access the Library shader kind. Library is the kind of the module. It does not identify the pipeline stage that reaches the access. PIX cannot tell whether a record comes from a ray generation, closest-hit, or miss shader.
The access record holds the instruction number in a field of 24 bits. A larger number overflows into the adjacent fields of the record.
PIX receives records for helper functions, and each record carries a pipeline stage. A tool that filtered out records with the Library kind will see more data.
Assisted-by: Copilot
Stack created with GitHub Stacks CLI • Give Feedback 💬