Fix CFP to avoid removing synchronization #7311
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
ConstantFieldPropagation previously reasoned that if a field has a constant value, then it is not possible for release-acquire operations to use it for synchronization, since the read can be assumed to be from a previous write. This logic was incorrect because reads must read the value written by the last write in the global modification order for the accessed location. In principle it would be possible for CFP to detect fields that have both constant values and also either no acquire reads or no release writes. Such fields cannot possibly be part of synchronization edges and their gets could be optimized normally. For now though, simply do not optimize any gets with acquire or seqcst ordering. Similarly, stop optimizing RMW operations because they can also form synchronization edges.
kripken
approved these changes
Feb 21, 2025
| // final type can be fairly fast (perhaps constant time). We therefore | ||
| // look if one of the sets of types contains a single type and it is | ||
| // final, and if so then we'll test on it. (However, see a few lines below | ||
| // on how we test for finality.) |
There was a problem hiding this comment.
Why did this change? It was 80-column conformant already.
There was a problem hiding this comment.
Oh, hmm, I had accidentally added some random characters in the middle of a word here. I removed them and reformatted, but I guess the formatter kept the line breaks from before I had removed them. Will fix.
test/lit/passes/cfp-rmw.wast
Outdated
Comment on lines
275
to
276
| ;; This cmpxchg copies the field, so does not change the value. It can still | ||
| ;; not be optimized. |
There was a problem hiding this comment.
Suggested change
| ;; This cmpxchg copies the field, so does not change the value. It can still | |
| ;; not be optimized. | |
| ;; This cmpxchg copies the field, so does not change the value. It still | |
| ;; cannot be optimized. |
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.
ConstantFieldPropagation previously reasoned that if a field has a
constant value, then it is not possible for release-acquire operations
to use it for synchronization, since the read can be assumed to be from
a previous write. This logic was incorrect because reads must read the
value written by the last write in the global modification order for the
accessed location.
In principle it would be possible for CFP to detect fields that have
both constant values and also either no acquire reads or no release
writes. Such fields cannot possibly be part of synchronization edges and
their gets could be optimized normally. For now though, simply do not
optimize any gets with acquire or seqcst ordering. Similarly, stop
optimizing RMW operations because they can also form synchronization
edges.