Open
Conversation
When non-escaping allocations flow into the `expected` field of an ArrayCmpxchg, optimize even if the ArrayCmpxchg has a non-constant index. We typically only optimize array instructions with constant fields, but that's because we need to know what field of the array is accessed. Arrays flowing into the `expected` field are not accessed, though, so there is no need for the accessed index to be constant. Make sure that effects in the potentially non-constant index field are preserved in the correct order by using a new scratch local to propagate the index value past the `expected` and `replacement` expressions to the newly generated `struct.atomic.get`.
kripken
reviewed
Apr 2, 2026
| return; | ||
| } | ||
| if (curr->ref == child || curr->expected == child) { | ||
| if (child == curr->expected || |
Member
There was a problem hiding this comment.
Worth a comment here I think, as in the PR description.
| return; | ||
| } | ||
|
|
||
| // The accessed array is being optimzied. Convert the ArrayCmpxchg into a |
Member
There was a problem hiding this comment.
Suggested change
| // The accessed array is being optimzied. Convert the ArrayCmpxchg into a | |
| // The accessed array is being optimized. Convert the ArrayCmpxchg into a |
(pre-existing, but while we are here)
| // The `expected` value must be getting optimized. Since the accessed object | ||
| // is remaining an array for now, do not change anything. The ArrayCmpxchg | ||
| // will be optimized later by Struct2Local. | ||
| return; |
Member
There was a problem hiding this comment.
IIUC this moves the trapping case into the handling of ref. But the case of expected should still trap, I think? I don't see that handled in the code that is reached later?
| (call $effect-i32) | ||
| (array.new_default $array (i32.const 1)) | ||
| (call $effect-eq) | ||
| ) |
Member
There was a problem hiding this comment.
Is there a test for array.atomic.rmw.cmpxchg where we optimize the expected field but the index is out of bounds?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When non-escaping allocations flow into the
expectedfield of an ArrayCmpxchg, optimize even if the ArrayCmpxchg has a non-constant index. We typically only optimize array instructions with constant fields, but that's because we need to know what field of the array is accessed. Arrays flowing into theexpectedfield are not accessed, though, so there is no need for the accessed index to be constant.Make sure that effects in the potentially non-constant index field are preserved in the correct order by using a new scratch local to propagate the index value past the
expectedandreplacementexpressions to the newly generatedstruct.atomic.get.