Add DebugBuildIdentifier to the live worklist during initialization#6618
Open
jkwak-work wants to merge 1 commit intoKhronosGroup:mainfrom
Open
Add DebugBuildIdentifier to the live worklist during initialization#6618jkwak-work wants to merge 1 commit intoKhronosGroup:mainfrom
jkwak-work wants to merge 1 commit intoKhronosGroup:mainfrom
Conversation
DebugBuildIdentifier was missing from the set of NonSemantic debug instructions added to the worklist in InitializeModuleScopeLiveInstructions. Instead it was handled in ProcessGlobalValues (after the worklist is exhausted) using live_insts_.Set() on its direct operands. live_insts_.Set() marks an instruction live but does not enqueue it, so transitive operand dependencies are never visited. In shaders where the only use of a type (e.g. OpTypeInt) is through a constant that is itself only referenced by DebugBuildIdentifier's flags argument, the type instruction is not marked live and is incorrectly killed by ADCE. The surviving constant then references a deleted type, producing invalid SPIR-V. Any subsequent pass that rebuilds the DefUseManager (e.g. CCP) will fail with "Definition is not registered." Fix: include NonSemanticShaderDebugInfo100DebugBuildIdentifier in the worklist loop alongside the other always-live debug instructions. The worklist's transitive closure correctly marks all operand dependencies live before global-value cleanup runs. The existing special-case code in ProcessGlobalValues becomes unreachable for this opcode (IsLive returns true, so the early continue fires) and is now dead; it is left in place for safety.
Author
|
I am not able to make a simple repro SPIRV for this issue. As far as I can tell the problem is on the fact that the constant variable used for DebugBuildIdentifier is not recognized and it causes an assertion failure as following: This issue was observed only with an option, If I can somehow make a simpler repro case later, I will share it to have it as a regression test. |
Author
|
I found a small repro Spirv Code for the issue. Fixes #6619 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DebugBuildIdentifier was missing from the set of NonSemantic debug instructions added to the worklist in InitializeModuleScopeLiveInstructions.
Instead it was handled in ProcessGlobalValues (after the worklist is
exhausted) using live_insts_.Set() on its direct operands.
live_insts_.Set() marks an instruction live but does not enqueue it, so transitive operand dependencies are never visited. In shaders where the only use of a type (e.g. OpTypeInt) is through a constant that is itself only referenced by DebugBuildIdentifier's flags argument, the type instruction is not marked live and is incorrectly killed by ADCE.
The surviving constant then references a deleted type, producing invalid SPIR-V. Any subsequent pass that rebuilds the DefUseManager (e.g. CCP) will fail with "Definition is not registered."
Fix: include NonSemanticShaderDebugInfo100DebugBuildIdentifier in the worklist loop alongside the other always-live debug instructions. The worklist's transitive closure correctly marks all operand dependencies live before global-value cleanup runs. The existing special-case code in ProcessGlobalValues becomes unreachable for this opcode (IsLive returns true, so the early continue fires) and is now dead; it is left in place for safety.