Do not optimize atomic gets in GUFA #7161
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
Conservatively avoid introducing synchronization bugs by not optimizing atomic struct.gets at all in GUFA. It is possible that we could be more precise in the future. Also remove obsolete logic dealing with the types of null values as a drive-by. All null values now have bottom types, so the type mismatch this code checked for is impossible.
kripken
approved these changes
Dec 19, 2024
src/ir/properties.h
Outdated
| // shared memory synchronization, return the memory order corresponding to the | ||
| // kind of synchronization it does. Return MemoryOrder::Unordered if there is no | ||
| // synchronization. Does not look at children. | ||
| inline MemoryOrder getSynchronization(Expression* curr) { |
There was a problem hiding this comment.
Why not getMemoryOrder since that is what it returns?
| // synchronize with that store anymore. Since we know what value the store | ||
| // must write, and we know it is the same as every other store to the same | ||
| // location, it's possible that optimizing here would be allowable, but | ||
| // for now be conservative and do not optimize. |
There was a problem hiding this comment.
Suggested change
| // for now be conservative and do not optimize. | |
| // for now (TODO) be conservative and do not optimize. |
test/lit/passes/gufa-refs.wast
Outdated
| (struct.new_default $A) | ||
| ) | ||
| (drop | ||
| ;; This is optmizable. |
There was a problem hiding this comment.
Suggested change
| ;; This is optmizable. | |
| ;; This is optimizable. |
test/lit/passes/gufa-refs.wast
Outdated
| ) | ||
| (drop | ||
| ;; This is optmizable. | ||
| ;; TODO: Should it not be optimizable since it reads from shared memory? |
There was a problem hiding this comment.
Suggested change
| ;; TODO: Should it not be optimizable since it reads from shared memory? |
I think this is optimizable. GUFA has seen that all writes to this type+field combination all write the same value, hence this field can only contain one value. That value might also be written from another thread, but that's fine.
kripken
approved these changes
Dec 19, 2024
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.
Conservatively avoid introducing synchronization bugs by not optimizing
atomic struct.gets at all in GUFA. It is possible that we could be more
precise in the future.
Also remove obsolete logic dealing with the types of null values as a
drive-by. All null values now have bottom types, so the type mismatch
this code checked for is impossible.