Fix debug info nondeterminism in LowerStaticGlobalIntoAlloca - #8899
Marijn Suijten (MarijnS95) wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
A regression test and release-note entry are required, and the new comment contains minor technical inaccuracies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces pointer-dependent iteration with insertion-ordered rewriting to make embedded debug information deterministic.
Changes:
- Uses
MapVectorfor deterministic global-use rewriting. - Documents why iteration order affects debug metadata.
File summaries
| File | Description |
|---|---|
lib/Transforms/Scalar/ScalarReplAggregatesHLSL.cpp |
Preserves instruction collection order during static-global lowering. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Squashed the branch to a single commit and rebased it onto current Net diff is unchanged and still three files: the |
Addresses review feedback on microsoft#8899. The test re-runs `static-global-to-alloca` on a fresh clone of the module via `opt -run-twice`, which allocates the instructions at different addresses and reports a difference between the two outputs as an error, so it fails on the unfixed pass without depending on which particular order that pass happened to produce. Based on the existing `local_global_inline_scope.ll`, which already covers this path with three users of the global and three inlined scopes; the RUN line is repeated as in `pointer-collision-non-determinism.ll` to shrink the chance of an ordering surviving both runs by coincidence. Also corrects the new comment: a use list is prepended to rather than appended to, and a DenseMap walks its buckets rather than ordering pointers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The regression test’s -run-twice flow operates on an already-transformed clone and therefore does not verify the reported nondeterminism.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
2f337a4 to
c47388e
Compare
lowerStaticGlobalIntoAlloca() collects the instructions using a static global into a DenseMap keyed on Instruction*, then iterates it to rewrite each use to the per-function alloca. A DenseMap walks its buckets, so that order is pointer-derived and moves with heap addresses from run to run. That used to be unobservable, but since microsoft#7799 added a DILocalVariable per inlined DISubprogram for these localized globals, PatchDebugInfo() walks AI->users() to decide which subprograms are relevant and in which order it creates that debug info. Rewriting the uses in pointer-derived order mutates AI's use list in that order, so the debug info comes out permuted. The result is a compiler that emits a different ILDB part for byte-identical input, twice in the same process: the same METADATA_LOCAL_VAR and DEBUG_LOC records with identical operands in a different order, and the metadata renumbering that follows. The DXIL part itself is unaffected, so this only breaks reproducible builds and content-addressed caching of shader artifacts, not codegen. Use a MapVector so the uses are rewritten in the order they were collected, which is the module's use-list order. The regression test runs the pass in separate processes and compares the outputs, as test/DXC/deterministic_output_resource_array.hlsl does, reusing the module from local_global_inline_scope.ll which already covers this path with three users of the global across three inlined scopes. That relies on the runs laying out the heap differently, so it detects the regression with high probability rather than with certainty; the test comment says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c47388e to
36c04a1
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The implementation directly removes pointer-derived iteration order, with regression coverage and release-note documentation included.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Chris B (llvm-beanz)
left a comment
Chris B (llvm-beanz)
left a comment
There was a problem hiding this comment.
LGTM.
In general for bugs like non-determinism I don't really look for a test case that exhibits the non-determinism (although that can be nice if possible), as much as much as verifying that we have tests that exercise the code and verify that it produces correct output.
DXC has a lot of gaps in test coverage, so adding a small bit of coverage for cases like this can add up over time to having better test coverage.
The HLSL SROA pass is one of my most hated parts of the DXC codebase because it is so undertested.
|
Chris B (@llvm-beanz) Yeah makes sense, I agree. Claude just found the requirement in |
lowerStaticGlobalIntoAlloca() collects the instructions using a static global into a DenseMap keyed on Instruction*, then iterates it to rewrite each use to the per-function alloca. A DenseMap walks its buckets, so that order is pointer-derived and moves with heap addresses from run to run. That used to be unobservable, but since microsoft#7799 added a DILocalVariable per inlined DISubprogram for these localized globals, PatchDebugInfo() walks AI->users() to decide which subprograms are relevant and in which order it creates that debug info. Rewriting the uses in pointer-derived order mutates AI's use list in that order, so the debug info comes out permuted. The result is a compiler that emits a different ILDB part for byte-identical input, twice in the same process: the same METADATA_LOCAL_VAR and DEBUG_LOC records with identical operands in a different order, and the metadata renumbering that follows. The DXIL part itself is unaffected, so this only breaks reproducible builds and content-addressed caching of shader artifacts, not codegen. Use a MapVector so the uses are rewritten in the order they were collected, which is the module's use-list order. The regression test runs the pass in separate processes and compares the outputs, as test/DXC/deterministic_output_resource_array.hlsl does, reusing the module from local_global_inline_scope.ll which already covers this path with three users of the global across three inlined scopes. That relies on the runs laying out the heap differently, so it detects the regression with high probability rather than with certainty; the test comment says so. Cherry-picked from MarijnS95:upstream-determinism-fix by Marijn Suijten (MarijnS95): microsoft#8899. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lowerStaticGlobalIntoAlloca()collects the instructions using a static global into aDenseMap<Instruction *, Value *>and then iterates it to rewrite each use onto the per-function alloca. That iteration order is pointer order, so it varies with heap addresses from run to run. The map has been aDenseMapsince #3350, but nothing observable depended on the rewrite order until #7799 started creating aDILocalVariableper inlinedDISubprogramfor these localized globals:PatchDebugInfo()walksAI->users()to decide which subprograms are relevant and in which order, and rewriting the uses in pointer order appends the replacement instructions to that use list in pointer order. So the debug info comes out permuted.The user-visible effect is that DXC emits a different
ILDBpart for byte-identical input, twice in the same process. We build a few thousand shader pipelines with-Zi -Qembed_debugand verify reproducibility by building every asset twice and comparing, and that check started failing on an arbitrary, run-dependent handful of them after we moved to v1.10.2605.37. Diffing the two containers part by part,ILDBis the only one that differs —DXIL,STAT,ISG1/OSG1/PSV0,ILDNandHASHare byte-identical — and putting both bitcode streams throughllvm-bcanalyzer --dumpyields dumps that are an exact permutation of one another: 92DEBUG_LOC, 12METADATA_LOCAL_VARand 4INST_CALLrecords with identical operands in a different order, plus the metadata renumbering that follows. Resolving the operands of the permutedLOCAL_VARrecords gives names likeglobal.kLowBoundandglobal.kL2ChildrenwithDW_TAG_arg_variableandDISubprogramscopes, which is exactly whatPatchDebugInfo()constructs. Codegen is unaffected, so this breaks reproducible builds and content-addressed caching of shader artifacts rather than the shaders themselves.Switching to a
MapVectorrewrites the uses in the order they were collected, which is the module's use-list order.MapVectoris already used elsewhere in this file. With a compiler built from this change, our check builds all 2503 pipelines clean three runs in a row, where before it failed every single run; I also confirmed the change does not perturb codegen by disassembling theDXILpart before and after, where the only differing record is the version string DXC stamps into the module.On testing, I could use some guidance rather than guessing. CONTRIBUTING asks for a test that fails without the non-test change, and a plain FileCheck over the emitted order would be unreliable here, because the order this produced was a function of heap layout and a single run could pass either way. Two existing patterns look applicable: the double-compile-and-
diffused bytools/clang/test/DXC/deterministic_output_resource_array.hlsl, oropt -run-twiceas intools/clang/test/DXC/Passes/reassociate/pointer-collision-non-determinism.ll, which exists for the same species of pointer-keyed-container bug and is the more deterministic of the two since the second run reallocates. I have not committed either, because I have no local build to validate that the test actually fails without the fix, and I would rather not add a test I have not seen fail. Happy to write whichever shape you prefer.Worth noting there is close precedent in this same file: #5259 fixed debug info nondeterminism in
SROAGlobalAndAllocasfrom stalestaticGVspointers, and its test comments already note that the scalarized variables are expected to appear in a deterministic order.PatchDebugInfo()is order-sensitive by construction, so anything that later feeds it an unordered container will reintroduce this.Finally, we consume v1.10.2605.37 rather than
main— would a backport to the correspondingrelease-preview-1.10.2605branch be possible once this lands, so we can drop the commit we are carrying?