ir: Fix incorrect renaming of phinode values #52614
Merged
Conversation
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
This fixes #52610. The underlying issue is a left over OldSSAValue after the adce_pass! (introduced by compaction, it being during adce is incidental). Compaction introduces `OldSSAValue` when it compacts in PhiNodes that reference later SSAValues and adds them to a list to revisit at the end of compaction to fill in the actual renamed result. There are two separate fixes here: 1. If the result of the final revisit is yet another `OldSSAValue`, rename it again. I don't this ordinarily happens at all, but I suppose it is possible in theory during sroa beacuse of the rename-shortcut optimization [1]. However, this is not not what happened here. Instead compaction incorrectly used an OldSSAValue for an already-inserted node, which then ends up in the rename list because we deleted one of the predecessor edges [2]. To fix that we: 2. Fix an issue where we weren't accounting for the possibility of previously pending nodes (which have SSAValues beyond the numbering range of the ordinary statements) in the special already_inserted query in phinode value processing. To fix this, unify the logic with the ordinary `already_inserted` query, which handles this case correctly. [1] https://github.com/JuliaLang/julia/blob/master/base/compiler/ssair/passes.jl#L1385 [2] https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/ir.jl#L1556 Co-authored-by: Tim Besard <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This fixes #52610. The underlying issue is a left over OldSSAValue after the adce_pass! (introduced by compaction, it being during adce is incidental). Compaction introduces
OldSSAValuewhen it compacts in PhiNodes that reference later SSAValues and adds them to a list to revisit at the end of compaction to fill in the actual renamed result. There are two separate fixes here:If the result of the final revisit is yet another
OldSSAValue, rename it again. I don't this ordinarily happens at all, but I suppose it is possible in theory during sroa beacuse of the rename-shortcut optimization [1]. However, this is not not what happened here. Instead compaction incorrectly used an OldSSAValue for an already-inserted node, which then ends up in the rename list because we deleted one of the predecessor edges [2]. To fix that we:Fix an issue where we weren't accounting for the possibility of previously pending nodes (which have SSAValues beyond the numbering range of the ordinary statements) in the special already_inserted query in phinode value processing. To fix this, unify the logic with the ordinary
already_insertedquery, which handles this case correctly.[1]
julia/base/compiler/ssair/passes.jl
Line 1385 in 9443c76
[2]
julia/base/compiler/ssair/ir.jl
Line 1556 in 9443c76