irverify: Enforce invariant that PhiNodes are at the beginning of a BB #50158
Conversation
This file contains 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
oxinabox
approved these changes
Jun 14, 2023
aviatesk
approved these changes
Jun 14, 2023
|
Just minor suggestions. I am very fine with merging this as is if you need this hurry. I can follow up it later if you want. |
We have an invariant that all PhiNodes are at the beginning of a BasicBlock (only possible interrupted by a `nothing`) and we rely on this in various places for correctness. However, we did not actually verify this invariant.
Keno
force-pushed
the
kf/irverifyphiblock
branch
from
01fd53f to
d8f2a67
Compare
Keno
added a commit
that referenced
this pull request
Jun 20, 2023
As of #50158, irverify catches cases where PhiNodes show up in the middle of a basic block (which is illegal). Unfortunately, it turns out there were two cases in Base, where we created just such code: 1. When cfg_simplify! merged basic blocks, it didn't bother to delete (resp, replace by the one incoming edge) the PhiNodes in the basic block it was merging. 2. In irinterp we try to delete instructions that result in constants. This is not legal if the instruction is a PhiNode. The second of these is somewhat unfortunate, but any subsequent compaction will of course take care of it, so I don't think it's a huge issue to just disable the replacement.
staticfloat
pushed a commit
that referenced
this pull request
Jun 20, 2023
As of #50158, irverify catches cases where PhiNodes show up in the middle of a basic block (which is illegal). Unfortunately, it turns out there were two cases in Base, where we created just such code: 1. When cfg_simplify! merged basic blocks, it didn't bother to delete (resp, replace by the one incoming edge) the PhiNodes in the basic block it was merging. 2. In irinterp we try to delete instructions that result in constants. This is not legal if the instruction is a PhiNode. The second of these is somewhat unfortunate, but any subsequent compaction will of course take care of it, so I don't think it's a huge issue to just disable the replacement.
Keno
added a commit
that referenced
this pull request
Jun 21, 2023
As of #50158, irverify catches cases where PhiNodes show up in the middle of a basic block (which is illegal). Unfortunately, it turns out there were two cases in Base, where we created just such code: 1. When cfg_simplify! merged basic blocks, it didn't bother to delete (resp, replace by the one incoming edge) the PhiNodes in the basic block it was merging. 2. In irinterp we try to delete instructions that result in constants. This is not legal if the instruction is a PhiNode. The second of these is somewhat unfortunate, but any subsequent compaction will of course take care of it, so I don't think it's a huge issue to just disable the replacement.
Keno
added a commit
that referenced
this pull request
Jun 27, 2023
In #50158, I tought the verifier to reject code that has invalid statements in the original PHI block. In #50235, this required irinterp to stop folding PhiNodes to the respective constants. I said at the time that a subsequent compact would fix it, but it turns out that we don't actually have the logic for that. I might still add that logic, but on the other hand it just seems kinda silly that PhiNodes need to be a special case here. This PR relaxes the semantics of the PHI block, to allow any value-position constant to appear in the PHI block and undoes the irinterp change from #50235. Only the interpreter really cares about the semantics of the phi block, so the primary change is there. Of note, SSAValue forwards are not allowed in the phi block. This is because of the following: ``` loop: %1 = %(...) %2 = %1 %3 = %(top => %1) ``` The two phi values %1 and %2 have different semantics: %1 gets the *current* iteration of the loop, while %3 gets the *previous* value. As a result, any pass that wants to move SSAValues out of PhiNode uses would have to be aware of these semantics anyway, and there's no simplicitly benefits to allowing SSAValues in the middle of a phi block.
Keno
added a commit
that referenced
this pull request
Jun 27, 2023
In #50158, I tought the verifier to reject code that has invalid statements in the original PHI block. In #50235, this required irinterp to stop folding PhiNodes to the respective constants. I said at the time that a subsequent compact would fix it, but it turns out that we don't actually have the logic for that. I might still add that logic, but on the other hand it just seems kinda silly that PhiNodes need to be a special case here. This PR relaxes the semantics of the PHI block, to allow any value-position constant to appear in the PHI block and undoes the irinterp change from #50235. Only the interpreter really cares about the semantics of the phi block, so the primary change is there. Of note, SSAValue forwards are not allowed in the phi block. This is because of the following: ``` loop: %1 = %(...) %2 = %1 %3 = %(top => %1) ``` The two phi values %1 and %2 have different semantics: %1 gets the *current* iteration of the loop, while %3 gets the *previous* value. As a result, any pass that wants to move SSAValues out of PhiNode uses would have to be aware of these semantics anyway, and there's no simplicitly benefits to allowing SSAValues in the middle of a phi block.
Keno
added a commit
that referenced
this pull request
Jun 27, 2023
In #50158, I tought the verifier to reject code that has invalid statements in the original PHI block. In #50235, this required irinterp to stop folding PhiNodes to the respective constants. I said at the time that a subsequent compact would fix it, but it turns out that we don't actually have the logic for that. I might still add that logic, but on the other hand it just seems kinda silly that PhiNodes need to be a special case here. This PR relaxes the semantics of the PHI block, to allow any value-position constant to appear in the PHI block and undoes the irinterp change from #50235. Only the interpreter really cares about the semantics of the phi block, so the primary change is there. Of note, SSAValue forwards are not allowed in the phi block. This is because of the following: ``` loop: %1 = %(...) %2 = %1 %3 = %(top => %1) ``` The two phi values %1 and %2 have different semantics: %1 gets the *current* iteration of the loop, while %3 gets the *previous* value. As a result, any pass that wants to move SSAValues out of PhiNode uses would have to be aware of these semantics anyway, and there's no simplicitly benefits to allowing SSAValues in the middle of a phi block.
Keno
added a commit
that referenced
this pull request
Jun 28, 2023
In #50158, I tought the verifier to reject code that has invalid statements in the original PHI block. In #50235, this required irinterp to stop folding PhiNodes to the respective constants. I said at the time that a subsequent compact would fix it, but it turns out that we don't actually have the logic for that. I might still add that logic, but on the other hand it just seems kinda silly that PhiNodes need to be a special case here. This PR relaxes the semantics of the PHI block, to allow any value-position constant to appear in the PHI block and undoes the irinterp change from #50235. Only the interpreter really cares about the semantics of the phi block, so the primary change is there. Of note, SSAValue forwards are not allowed in the phi block. This is because of the following: ``` loop: %1 = %(...) %2 = %1 %3 = %(top => %1) ``` The two phi values %1 and %2 have different semantics: %1 gets the *current* iteration of the loop, while %3 gets the *previous* value. As a result, any pass that wants to move SSAValues out of PhiNode uses would have to be aware of these semantics anyway, and there's no simplicitly benefits to allowing SSAValues in the middle of a phi block.
Keno
added a commit
that referenced
this pull request
Jun 29, 2023
In #50158, I tought the verifier to reject code that has invalid statements in the original PHI block. In #50235, this required irinterp to stop folding PhiNodes to the respective constants. I said at the time that a subsequent compact would fix it, but it turns out that we don't actually have the logic for that. I might still add that logic, but on the other hand it just seems kinda silly that PhiNodes need to be a special case here. This PR relaxes the semantics of the PHI block, to allow any value-position constant to appear in the PHI block and undoes the irinterp change from #50235. Only the interpreter really cares about the semantics of the phi block, so the primary change is there. Of note, SSAValue forwards are not allowed in the phi block. This is because of the following: ``` loop: %1 = %(...) %2 = %1 %3 = %(top => %1) ``` The two phi values %1 and %2 have different semantics: %1 gets the *current* iteration of the loop, while %3 gets the *previous* value. As a result, any pass that wants to move SSAValues out of PhiNode uses would have to be aware of these semantics anyway, and there's no simplicitly benefits to allowing SSAValues in the middle of a phi block.
kpamnany
pushed a commit
to RelationalAI/julia
that referenced
this pull request
Feb 27, 2024
JuliaLang#50158) We have an invariant that all PhiNodes are at the beginning of a BasicBlock (only possible interrupted by a `nothing`) and we rely on this in various places for correctness. However, we did not actually verify this invariant.
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.
We have an invariant that all PhiNodes are at the beginning of a BasicBlock (only possible interrupted by a
nothing) and we rely on this in various places for correctness. However, we did not actually verify this invariant.Found by @staticfloat and @oxinabox. Note that the diff is only a handful of lines, but there's some indentation change, so it looks larger.