Fix Array2Struct missing handlers for ArrayRMW and ArrayCmpxchg #8305
+185
−1
Conversation
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
The escape analysis in Heap2Local accepted ArrayRMW and ArrayCmpxchg as FullyConsumes, allowing non-escaping arrays with these operations through to the Array2Struct transformation. However, Array2Struct had no visitArrayRMW or visitArrayCmpxchg handlers, so these operations were left unrewritten after the array-to-struct conversion, producing broken IR where array operations referenced a now-nonexistent array allocation. This caused the operations to be silently replaced with unreachable blocks, turning working code into unconditional traps. Fix by: 1. Adding const-index checks to the escape analysis for ArrayRMW and ArrayCmpxchg (matching the existing checks for ArrayGet and ArraySet), since Array2Struct requires compile-time-known indices. 2. Adding visitArrayRMW and visitArrayCmpxchg to Array2Struct that convert array atomic operations to their struct equivalents (following the pattern of visitArrayGet and visitArraySet), with proper OOB handling.
0b2347e to
e4d2f12
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
The escape analysis in Heap2Local accepted
ArrayRMWandArrayCmpxchgasFullyConsumes, allowing non-escaping arrays with these operations through to theArray2Structtransformation. However,Array2Structhad novisitArrayRMWorvisitArrayCmpxchghandlers. This caused these operations to be left unrewritten after the array-to-struct conversion, and the type rewriting infrastructure replaced them with unreachable blocks — silently turning working code into unconditional traps.Before fix (array.atomic.rmw.add that should return old value becomes unreachable):
Additionally, the escape analysis for
ArrayRMW/ArrayCmpxchgwas missing the constant-index check thatArrayGet/ArraySetboth have, whichArray2Structrequires to map array elements to struct fields.Fix
!curr->index->is<Const>()checks to the escape analysis forArrayRMWandArrayCmpxchg(matchingArrayGet/ArraySet)visitArrayRMWandvisitArrayCmpxchgtoArray2Structthat convert array atomic operations to their struct equivalents, following the pattern ofvisitArrayGet/visitArraySetTest plan
test/lit/passes/heap2local-rmw.wast:$array-rmw-add: array.atomic.rmw.add on array.new_fixed — fully optimized to locals$array-cmpxchg: array.atomic.rmw.cmpxchg on array.new_fixed — fully optimized to locals$array-rmw-nonconstant-index: non-constant index prevents optimization (left as-is)