[Strings] Remove stringview types and instructions #6579

tlively · 2024-05-10T05:21:14Z

The stringview types from the stringref proposal have three irregularities that
break common invariants and require pervasive special casing to handle properly:
they are supertypes of none but not subtypes of any, they cannot be the
targets of casts, and they cannot be used to construct nullable references. At
the same time, the stringref proposal has been superseded by the imported
strings proposal, which does not have these irregularities. The cost of
maintaing and improving our support for stringview types is no longer worth the
benefit of supporting them.

Simplify the code base by entirely removing the stringview types and related
instructions that do not have analogues in the imported strings proposal and do
not make sense in the absense of stringviews.

Three remaining instructions, stringview_wtf16.get_codeunit,
stringview_wtf16.slice, and stringview_wtf16.length take stringview operands
in the stringref proposal but cannot be removed because they lower to operations
from the imported strings proposal. These instructions are changed to take
stringref operands in Binaryen IR, and to allow a graceful upgrade path for
users of these instructions, the text parser still accepts but ignores
string.as_wtf16, which is the instruction used to convert stringrefs to
stringviews.

No attempt is made to fix up the binary output to include the string.as_wtf16
instructions that the stringref proposal requires, so binaryen no longer emits
valid string code unless --string-lowering is used to target the imported
strings proposal instead. This should not be a problem because users should
universally be using imported strings over stringref.

Future PRs will further align binaryen with the imported strings proposal
instead of the stringref proposal, for example by making string a subtype of
extern instead of a subtype of any and by removing additional instructions
that do not have analogues in the imported strings proposal.

tlively · 2024-05-10T05:21:25Z

[Strings] Remove operations not included in imported strings #6589
[Strings] Remove stringview types and instructions #6579 👈
Simplify scratch local calculation #6583
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlively and the rest of your teammates on Graphite

tlively · 2024-05-10T05:49:09Z

The fuzzer won't be happy with this until it supports --string-lowering and/or --string-lowering-magic-imports and we can force one of those passes to be run whenever strings are enabled.

Alternatively, we could start performing string lowering automatically in the binary writer since we no longer emit valid stringref code anway.

Alternatively, we could actually try to emit the necessary string.as_wtf16 instruction in the binary writer so we still emit valid stringref code. I was hoping to avoid this complication, but it's probably the best solution for now. I'll investigate this more tomorrow.

tlively · 2024-05-10T16:24:36Z

Injecting string.as_wtf16 in the binary writer requires a lot of the same infrastructure you're working on to fix up br_if, so I think we should either land this after that infrastructure lands, or land this sooner and accept temporary fuzzer breakage.

tlively · 2024-05-12T07:55:33Z

Never mind, the last commit now implements injecting the conversions into the binary along with the necessary scratch locals.

kripken · 2024-05-13T19:31:06Z

I do think it's important for now to emit validating strings logic as you mention the latest version does, as we can't fully fuzz lowered strings logic yet - we'd need to handle the split-out constants and maybe other stuff.

kripken · 2024-05-13T19:33:55Z

There is a risk here if the strings proposal starts back up again. I would not be entirely surprised if it does. In that case we may need to restore some of this, but given V8 has been making the changes (like non-nullability) that led to this PR, I guess whatever new form a strings proposal would take would be different anyhow.

kripken · 2024-05-13T19:35:28Z

src/parser/wat-parser.cpp

+        auto end = ctx.irBuilder.visitEnd();
+        if (auto* err = end.getErr()) {
+          return ctx.in.err(decls.funcDefs[i].pos, err->msg);
+        }


What does this change do?

Adds a source location to the error message. Otherwise you just get the plain error message where no indication of where the error occurred.

I see, thanks. Is there a particular reason it is needed here and not elsewhere? (This location doesn't stand out to me.)

In general the errors returned from parser methods already contain source locations because they were generated with Lexer::err in the first place. This error comes from IRBuilder, though, so it doesn't know anything about source locations and we have to add one.

kripken · 2024-05-13T19:36:44Z

src/tools/fuzzing/fuzzing.cpp

 Expression* TranslateToFuzzReader::makeStringSlice() {
-  // StringViews cannot be non-nullable.
-  auto* ref = make(Type(HeapType::stringview_wtf16, NonNullable));
+  auto* ref = make(Type(HeapType::string, getNullability()));


All these should be makeTrappingRefUse. That reduces the frequency of null traps.

src/wasm/wasm-binary.cpp

kripken · 2024-05-13T19:53:56Z

src/wasm/wasm-stack.cpp

+        return;
+      }
+      auto& count = scratches[Type::i32];
+      count = std::max(count, 1u);


Maybe add a comment here, and especially below for the case of 2u, what the scratch locals are for (below, "one for the start and one for the end" or such).

kripken · 2024-05-13T19:55:32Z

test/binaryen.js/kitchen-sink.js.txt

+StringIterNext: undefined
+StringIterMove: undefined
+StringSliceWTF: 86
+StringSliceIter: undefined


The undefineds could be removed.

kripken · 2024-05-13T19:57:12Z

test/lit/exec/strings.wast

-  ;; CHECK-NEXT: [fuzz-exec] note result: get_length => 7
-  (func $get_length (export "get_length") (result i32)
-    ;; This should return 7.
-    (stringview_wtf16.length


Maybe keep this but use the non-view length operation? I'm not sure we have coverage otherwise.

We have coverage in string.as_wtf16.wast for this instruction and we do already have coverage of string.measure_wtf16 in this file as well.

test/lit/string.as_wtf16.wast

kripken · 2024-05-13T20:01:18Z

test/lit/string.as_wtf16.wast

+    ;; This should parse ok with the conversion skipped. The roundtrip will
+    ;; include scratch locals.
+    (stringview_wtf16.get_codeunit
+      (string.as_wtf16


It is also valid to write this without this line, correct? Is the output different somehow in that case? Worth testing either way I think.

Yes, I'll add both in the input.

Actually, we already have coverage up in the empty case that shows that the string.as_wtf16 is ignored.

test/lit/string.as_wtf16.wast

The stringview types from the stringref proposal have three irregularities that break common invariants and require pervasive special casing to handle properly: they are supertypes of `none` but not subtypes of `any`, they cannot be the targets of casts, and they cannot be used to construct nullable references. At the same time, the stringref proposal has been superseded by the imported strings proposal, which does not have these irregularities. The cost of maintaing and improving our support for stringview types is no longer worth the benefit of supporting them. Simplify the code base by entirely removing the stringview types and related instructions that do not have analogues in the imported strings proposal and do not make sense in the absense of stringviews. Three remaining instructions, `stringview_wtf16.get_codeunit`, `stringview_wtf16.slice`, and `stringview_wtf16.length` take stringview operands in the stringref proposal but cannot be removed because they lower to operations from the imported strings proposal. These instructions are changed to take stringref operands in Binaryen IR, and to allow a graceful upgrade path for users of these instructions, the text parser still accepts but ignores `string.as_wtf16`, which is the instruction used to convert stringrefs to stringviews. No attempt is made to fix up the binary output to include the `string.as_wtf16` instructions that the stringref proposal requires, so binaryen no longer emits valid string code unless `--string-lowering` is used to target the imported strings proposal instead. This should not be a problem because users should universally be using imported strings over stringref. Future PRs will further align binaryen with the imported strings proposal instead of the stringref proposal, for example by making `string` a subtype of `extern` instead of a subtype of `any` and by removing additional instructions that do not have analogues in the imported strings proposal.

So that we continue to produce valid stringref code. Parse the conversions as nops in the binary parser to preserve our ability to round-trip. Emitting the conversions requires using extra scratch locals, and between that and parsing the nop, there is quite a bit of code bloat when round-tripping, but stringref code should never be emitted in production use cases, so that's ok.

kripken

lgtm % question

kripken · 2024-05-15T04:02:07Z

src/wasm/wasm-stack-opts.cpp

+  // fewer scratch locals when their operands are already LocalGets. To avoid
+  // interfering with that optimization, we have to avoid removing such
+  // LocalGets.
+  auto deferredGets = findStringViewDeferredGets();


I'm not that happy with this change. But it is pretty small at least.

I wonder if maybe just disabling StackIR opts entirely when there is a relevant string view operation would be simpler?

I was also unable to construct a test case that actually exercised this code; maybe it's impossible, in which case we can just not add any code here, which would be nice.

I'll try turning this code into an assertion instead of a check and see if the fuzzer can come up with a test case for me.

Ok yes, it turns out all I was missing was --shrink-level=3 and the existing test is enough to exercise the new check in stack IR.

I wonder if maybe just disabling StackIR opts entirely when there is a relevant string view operation would be simpler?

Maybe, although the code we would save doesn't do much more than find the stringview ops itself. I propose we leave it as-is for now. I'm sure at some point we will do some kind of further refactoring with stack IR, so perhaps we can clean it up then.

Fair enough, sgtm to land.

I do hope we can avoid worrying about this kind of thing as modify binary writing logic. Maybe an option is to always skip StackIR if there is even a single scratch local, as that is rare anyhow. Well, we can think about it later.

kripken

lgtm with fuzzing

tlively · 2024-05-15T19:05:07Z

Merge activity

May 15, 3:05 PM EDT: @tlively started a stack merge that includes this pull request via Graphite.
May 15, 3:05 PM EDT: @tlively merged this pull request with Graphite.

kripken · 2024-06-03T18:44:45Z

@tlively The fuzzer found a regression from this PR:

(module
 (func $test (export "test") (result stringref)
  (local $0 i32)
  ;; Slice [0:1), which returns "h".
  (stringview_wtf16.slice
   (string.const "hello")
   (local.get $0)
   (local.tee $0
    (i32.const 1)
   )
  )
 )
)

After this landed, the following errors:

$ bin/wasm-opt w.wat -all --fuzz-exec --roundtrip
[fuzz-exec] calling test
[fuzz-exec] note result: test => string("h")
[fuzz-exec] calling test
[fuzz-exec] note result: test => string("")
[fuzz-exec] comparing test
values not identical! string("") != string("h")
[fuzz-exec] optimization passes changed results

tlively · 2024-06-11T18:51:34Z

Looking into this now.

tlively requested a review from kripken May 10, 2024 05:21

tlively mentioned this pull request May 10, 2024

Make stringview reference type shorthands non-nullable #6578

Closed

tlively force-pushed the no-stringviews branch 2 times, most recently from c6e413e to ab9bb22 Compare May 10, 2024 05:26

tlively force-pushed the no-stringviews branch from ab9bb22 to fa21b74 Compare May 10, 2024 05:50

tlively changed the base branch from stringviews-nonnullable to simplify-scratch-locals May 12, 2024 07:52

tlively force-pushed the no-stringviews branch from fa21b74 to 3858fe5 Compare May 12, 2024 07:52

tlively mentioned this pull request May 12, 2024

Simplify scratch local calculation #6583

Merged

tlively force-pushed the no-stringviews branch from 3858fe5 to 2c22eac Compare May 13, 2024 17:31

kripken reviewed May 13, 2024

View reviewed changes

tlively force-pushed the no-stringviews branch from 0d79c31 to 7b53017 Compare May 13, 2024 22:02

Base automatically changed from simplify-scratch-locals to main May 14, 2024 00:10

tlively added 3 commits May 13, 2024 17:21

address comments

03ffb8e

tlively force-pushed the no-stringviews branch from 7b53017 to 03ffb8e Compare May 14, 2024 00:22

tlively requested a review from kripken May 14, 2024 03:11

tlively changed the title ~~Remove stringview types and instructions~~ [Strings] Remove stringview types and instructions May 14, 2024

tlively mentioned this pull request May 14, 2024

[Strings] Remove operations not included in imported strings #6589

Merged

tlively added 3 commits May 14, 2024 16:39

optimize gets

ed26217

ignore string.as_wtf16 in binary

3705c4f

fix

4f09566

tlively mentioned this pull request May 15, 2024

Fix binary emitting of br_if with a refined value by emitting a cast #6510

Merged

kripken reviewed May 15, 2024

View reviewed changes

shrink level 3 to exercise stack ir changes

8fa2cc9

kripken approved these changes May 15, 2024

View reviewed changes

tlively merged commit ef4b57c into main May 15, 2024

tlively deleted the no-stringviews branch May 15, 2024 19:05

gkdn mentioned this pull request Aug 31, 2024

stringconsts gkdn/binaryen#1

Closed

[Strings] Remove stringview types and instructions #6579

[Strings] Remove stringview types and instructions #6579

Uh oh!

Conversation

tlively commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented May 10, 2024

Uh oh!

tlively commented May 10, 2024

Uh oh!

tlively commented May 12, 2024

Uh oh!

kripken commented May 13, 2024

Uh oh!

kripken commented May 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

tlively commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

kripken commented Jun 3, 2024

Uh oh!

tlively commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tlively commented May 10, 2024 •

edited

Loading

tlively commented May 10, 2024 •

edited

Loading

tlively commented May 15, 2024 •

edited

Loading