Relax constraints on the PHI block #50308
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
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
Nov 28, 2023
This restriction has been in there since this code was added in #44557. Unfortunately, I can't tell from the commit history why this restriction was added. It's possible that it was trying to avoid putting things into statement position that were not allowed in the phi block, but we have cleaned that up since (#50308 and related), so let's see if this restriction is still required, since I was seeing some suboptimial optimization results because of this.
aviatesk
pushed a commit
that referenced
this pull request
Dec 1, 2023
This restriction has been in there since this code was added in #44557. Unfortunately, I can't tell from the commit history why this restriction was added. It's possible that it was trying to avoid putting things into statement position that were not allowed in the phi block, but we have cleaned that up since (#50308 and related), so let's see if this restriction is still required, since I was seeing some suboptimial optimization results because of this.
Keno
added a commit
that referenced
this pull request
Dec 7, 2023
…52338) This restriction has been in there since this code was added in #44557. Unfortunately, I can't tell from the commit history why this restriction was added. It's possible that it was trying to avoid putting things into statement position that were not allowed in the phi block, but we have cleaned that up since (#50308 and related), so let's see if this restriction is still required, since I was seeing some suboptimial optimization results because of this. --------- Co-authored-by: Shuhei Kadowaki <[email protected]>
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.
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:
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.