Avoid creating invalid PhiNodes in IR passes #50235
Merged
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
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.
oscardssmith
added
bugfix
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.
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.
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:
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.
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.