Require specific array types for string ops #7810
Open
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 operations for creating strings from arrays and decoding strings into arrays previously accepted any array types with i8 or i16 elements, depending on the specific operation. This violates the principal types properties of the WebAssembly type system and was a bug in the design of the defunct stringref proposal. In practice, these operations are only useful when lowered to calls to the standard JS string builtins, which have strict requirements about what array types they accept. Reflect those requirements by restricting the array types we allow to be used with the string Expressions in our IR. This allows us to remove the special casing for these operations in ChildTyper. Since Precompute only worked on string operations taking immutable arrays, but those arrays are now invalid to use with string operations, let it precompute on mutable arrays in the special case where the array allocation is an immediate child of the string operation so it is known not to escape elsewhere. Hopefully this is enough to prevent serious optimization regressions.
kripken
approved these changes
Aug 8, 2025
| // Certain heap types are used by standard operations. Provide central accessors | ||
| // for them to avoid having to build them everywhere they are used. | ||
| HeapType getMutI8Array(); | ||
| HeapType getMutI16Array(); |
There was a problem hiding this comment.
Could this be in ir/array*.h perhaps?
There was a problem hiding this comment.
This will eventually get more types, not all of which are arrays. In particular I'm thinking of (type (struct (field (mut i32)))), which we will need for waitqueueref.
src/passes/Precompute.cpp
Outdated
| @@ -213,23 +213,23 @@ class PrecomputingExpressionRunner | |||
|
|
|||
| Flow visitStringNew(StringNew* curr) { | |||
| if (curr->op != StringNewWTF16Array) { | |||
| // TODO: handle other string ops. For now we focus on JS-like strings. | |||
| // TOOD: handle other string ops. For now we focus on JS-like strings. | |||
There was a problem hiding this comment.
Suggested change
| // TOOD: handle other string ops. For now we focus on JS-like strings. | |
| // TODO: handle other string ops. For now we focus on JS-like strings. |
src/passes/Precompute.cpp
Outdated
| @@ -213,23 +213,23 @@ class PrecomputingExpressionRunner | |||
|
|
|||
| Flow visitStringNew(StringNew* curr) { | |||
| if (curr->op != StringNewWTF16Array) { | |||
| // TODO: handle other string ops. For now we focus on JS-like strings. | |||
| // TOOD: handle other string ops. For now we focus on JS-like strings. | |||
| return Flow(NONCONSTANT_FLOW); | |||
| } | |||
|
|
|||
| // string.encode_wtf16_array is effectively an Array read operation, so | |||
There was a problem hiding this comment.
This comment mentions string.encode, but in string.new - looks like it might have been copy-pasted from StringEncode, below? Might as well fix it here.
| type.getArray().element == array16Element) { | ||
| updates[type] = array16; | ||
| } | ||
| } |
There was a problem hiding this comment.
No, because now it's no longer possible for any array operation to take a reference to an array type other than the one this code is introducing. Basically we're requiring that this replacement would be a no-op by construction.
| ;; CHECK-NEXT: ) | ||
| (func $string.new-immutable (result externref) | ||
| ;; This array is immutable and we can optimize here. | ||
| (func $string.new-mutable-indirect (result externref) | ||
| (string.new_wtf16_array |
There was a problem hiding this comment.
Please add a comment comparing to the above.
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 operations for creating strings from arrays and decoding strings
into arrays previously accepted any array types with i8 or i16 elements,
depending on the specific operation. This violates the principal types
properties of the WebAssembly type system and was a bug in the design of
the defunct stringref proposal.
In practice, these operations are only useful when lowered to calls to
the standard JS string builtins, which have strict requirements about
what array types they accept. Reflect those requirements by restricting
the array types we allow to be used with the string Expressions in our
IR. This allows us to remove the special casing for these operations in
ChildTyper.
Since Precompute only worked on string operations taking immutable
arrays, but those arrays are now invalid to use with string operations,
let it precompute on mutable arrays in the special case where the array
allocation is an immediate child of the string operation so it is known
not to escape elsewhere. Hopefully this is enough to prevent serious
optimization regressions.