opt: prevent meld to merge block with MaximalReconvergence #5557
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
The extension SPV_KHR_maximal_reconvergence adds more constraints around the merge blocks, and how the control flow can be altered. The one we address here is explained in the following part of the spec: Note: This means that the instructions in a break block will execute as if they were still diverged according to the loop iteration. This restricts potential transformations an implementation may perform on the IR to match shader author expectations. Similarly, instructions in the loop construct cannot be moved into the continue construct unless it can be proven that invocations are always converged. Until the optimizer is clever enough to determine if the invocation have already converged, we shall not meld a block which branches to a merge block into it, as it might move some instructions outside of the convergence region. This behavior being only required with the extension, this commit behavior change is gated by the extension. This means using wave operations without the maximal reconvergence extension might lead to undefined behaviors.
sudonatalie
approved these changes
Feb 5, 2024
Co-authored-by: Natalie Chouinard <chouinard.nm@gmail.com>
Co-authored-by: Natalie Chouinard <chouinard.nm@gmail.com>
s-perron
reviewed
Feb 6, 2024
| // cannot be moved into the continue construct unless it can be proven that | ||
| // invocations are always converged. | ||
| if (succ_is_merge && context->get_feature_mgr()->HasExtension( | ||
| kSPV_KHR_maximal_reconvergence)) { |
There was a problem hiding this comment.
I wonder if we should do this all of the time. Is there a reason not to?
There was a problem hiding this comment.
Maybe we should disallow merging with unresolve comments so auto-merge don't betrays us.
Well, the main reason is it is not disallowed by the spec. So I wanted to be conservative in the changes we do.
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.
The extension SPV_KHR_maximal_reconvergence adds more constraints around the merge blocks, and how the control flow can be altered.
The one we address here is explained in the following part of the spec:
Until the optimizer is clever enough to determine if the invocation have already converged, we shall not meld a block which branches to a merge block into it, as it might move some instructions outside of the convergence region.
This behavior being only required with the extension, this commit behavior change is gated by the extension.
This means using wave operations without the maximal reconvergence extension might lead to undefined behaviors.
Related to microsoft/DirectXShaderCompiler#6222