Repository navigation
[Strings] Handle fromCharCode and cast imports in StringLifting/Lowering - #9373
jeremy-code wants to merge 6 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support and optimizations for fromCharCode and cast operations in both the StringLifting and StringLowering passes. In StringLifting, fromCharCode is mapped to fromCodePoint with a 0xFFFF mask, and cast is lifted to a ref.cast to string. In StringLowering, casts to string are lowered to imported cast calls, and fromCodePoint with a 0xFFFF mask is optimized back to fromCharCode. The feedback suggests optimizing the lowering of nullable casts by skipping the redundant null check when the input operand is already known to be non-nullable.
StringLifting: fromCharCode(x) becomes string.from_code_point(x & 0xFFFF) and cast becomes non-nullable ref.cast to string. StringLowering: string.from_code_point(x & 0xFFFF) or equivalent becomes fromCharCode(x) and non-nullable ref.cast to string becomes cast(). Nullable ref.cast to string checks ref.is_null beforehand.
…-instructions.wast Co-authored-by: Thomas Lively <tlively123@gmail.com>
c26b4b0 to
e55e999
Compare
…st behavior Add a StringNewFromCharCode IR operation. The equivalent stringref operation is string.from_code_point where the codePoint is masked (x & 0xFFFF). Allow ref.cast with strings when strings are enabled, otherwise GC is required. StringLifting: Refinalize IR whenever a call has been replaced (i.e. externref -> stringref). StringLowering: Move replaceInstructions before updateTypes and add visitRefCast in Replacer.
| if (curr->op == StringNewFromCharCode) { | ||
| // This is not part of the spec, so use a bitmask to get the expected output | ||
| // String.fromCharCode(0x10000) => '\x00' | ||
| // String.fromCodePoint(0x10000) => '𐀀' | ||
| // Hence, String.fromCharCode(x) == String.fromCodePoint(x & 0xFFFF) | ||
| o << int8_t(BinaryConsts::I32Const) << S32LEB(0xFFFF) | ||
| << int8_t(BinaryConsts::I32And); | ||
| } |
There was a problem hiding this comment.
Rather than try to emit valid stringref instructions here, let's just invent a new opcode for this operation. The stringref proposal is not active, so we don't need to worry about compatibility, and using a dedicated opcode will prevent these masks from bloating the IR when we round-trip through a binary.
Does that sound reasonable to you, too, @kripken?
There was a problem hiding this comment.
I don't feel strongly either way. My hope is that we end up with a toolchain-level spec at some point (c.f. the bignums discussion). In that case, we would write a new spec and can add this new instruction then, I suppose; but I'd be ok the other way too (keeping to the current spec). Either seem fine. So if you prefer a new instruction, sgtm.
There was a problem hiding this comment.
Should I mention this in the README for the Binaryen IR? Not sure if that would be excessively verbose.
Binaryen supports a
stringreftype. This is similar to the currently-inactive [stringref proposal] except that the string type is a subtype ofexternrefrather thananyrefand that it has afromCharCodeoperation.
| bool hasStringCasts = | ||
| curr->type.isRef() && | ||
| curr->type.getHeapType().isMaybeShared(HeapType::string); | ||
| shouldBeTrue(getModule()->features.hasGC() || | ||
| (hasStringCasts && getModule()->features.hasStrings()), |
There was a problem hiding this comment.
| bool hasStringCasts = | |
| curr->type.isRef() && | |
| curr->type.getHeapType().isMaybeShared(HeapType::string); | |
| shouldBeTrue(getModule()->features.hasGC() || | |
| (hasStringCasts && getModule()->features.hasStrings()), | |
| bool isStringCast = curr->type.isString(); | |
| shouldBeTrue(getModule()->features.hasGC() || | |
| (isStringCast && getModule()->features.hasStrings()), |
I wonder if we will need to allow casts to nullexternref without GC enabled as well. Can you run the fuzzer (scripts/monitor_fuzz.py) to find out?
Please also add validation tests for this new special case.
There was a problem hiding this comment.
I think the fuzzer is set to disable strings whenever GC is disabled:
Line 2922 in 0c9067a
|
|
||
| ;; CHECK: (import "wasm:js-string" "substring" (func $substring (type $11) (param externref i32 i32) (result (ref extern)))) | ||
|
|
||
| ;; CHECK: (import "wasm:js-string" "fromCharCode" (func $fromCharCode_14 (type $0) (param i32) (result (ref extern)))) |
There was a problem hiding this comment.
I guess it's probably out of scope for this PR, but it seems bad that we end up with duplicate imports after the round trip.
Co-authored-by: Thomas Lively <tlively123@gmail.com>
Co-authored-by: Thomas Lively <tlively123@gmail.com>
…ble or string bottom type to be cast without GC StringFromCharCode now has opcode 0xab. Support unreachable casts and casts to the string bottom type (noextern) without requiring GC.
StringLifting: fromCharCode(x) becomes string.from_code_point(x & 0xFFFF) and cast becomes non-nullable ref.cast to string.
StringLowering: string.from_code_point(x & 0xFFFF) or equivalent becomes fromCharCode(x) and non-nullable ref.cast to string becomes cast(). Nullable ref.cast to string checks ref.is_null beforehand.