[Strings] Add a string-builtins feature, and lift/lower automatically when enabled #7601

kripken · 2025-05-16T00:00:43Z

This makes string optimizations happen automatically when
--enable-string-builtins (or -all).

The lifting/lowering happen globally, at optimal times in the
pipeline, so even -O3 -O3 -O3 will only lift once and lower once,
avoiding overhead as in #7540 which this replaces.

TODO: document in optimizer cookbook

tlively

Nice!

tlively · 2025-05-16T00:07:26Z

src/tools/optimization-options.h

+        PassRunner::Ordering ordering;
+        ordering.first = (i == firstDefault);
+        ordering.last = (i == lastDefault);


Alternatively:

Suggested change

PassRunner::Ordering ordering;

ordering.first = (i == firstDefault);

ordering.last = (i == lastDefault);

PassRunner::Ordering ordering{i == firstDefault, i == lastDefault};

I kind of like writing out which is first and which is last? I guess the natural order is (first, last) but it is still easier to read I think.

tlively · 2025-05-16T00:09:42Z

test/lit/string-builtins.wast

+
+;; RUN: foreach %s %t wasm-opt -O2 --enable-reference-types -S -o - | filecheck %s --check-prefix=MVP
+;; RUN: foreach %s %t wasm-opt -O2 -all -S -o - | filecheck %s --check-prefix=ALL
+;; RUN: foreach %s %t wasm-opt -O2 --enable-reference-types --enable-string-builtins -S -o - | filecheck %s --check-prefix=ESB


If you enable GC here, the printed output should be the same as with -all, so you could use one fewer check prefix.

Good idea, done.

kripken · 2025-05-16T14:55:18Z

Hmm, this PR fails many tests because of these TODOs for typed continuations:

binaryen/src/ir/subtype-exprs.h

Lines 415 to 423 in f8531ef

    
           void visitContNew(ContNew* curr) { WASM_UNREACHABLE("not implemented"); } 
        
           void visitContBind(ContBind* curr) { WASM_UNREACHABLE("not implemented"); } 
        
           void visitSuspend(Suspend* curr) { WASM_UNREACHABLE("not implemented"); } 
        
           void visitResume(Resume* curr) { WASM_UNREACHABLE("not implemented"); } 
        
           void visitResumeThrow(ResumeThrow* curr) { 
        
             WASM_UNREACHABLE("not implemented"); 
        
           } 
        
           void visitStackSwitch(StackSwitch* curr) { 
        
             WASM_UNREACHABLE("not implemented");

This problem becomes noticeable in this PR because we have tests that use -all on continuations code, and now we run StringLowering, which uses SubtypingDiscoverer, so we try to operate on those instructions.

So this PR is blocked on those TODOs.

tlively · 2025-05-16T17:05:21Z

I can take a look at those TODOs later today, unless you plan on working on them first.

kripken · 2025-05-16T17:21:08Z

Thanks, I don't think I'd have time today myself.

Now that `string` is a subtype of `extern`, the null type for strings and externrefs is the same, so we no longer need to fix up nulls in StringLowering. Unblocks #7601.

tlively · 2025-05-19T20:23:19Z

test/lit/basic/extra-branch-values.wast

Do you know why the type indices are changing here? Are we actually emitting more types in the output?

I think we are just running a type-rewriting pass we weren't before, leading to different sorting of the names.

tlively · 2025-05-19T20:40:52Z

src/passes/pass.cpp

+  // between.
+  if (wasm->features.hasStringBuiltins() && wasm->features.hasGC() &&
+      options.optimizeLevel >= 2 && ordering.last) {
+    addIfNoDWARFIssues("string-lowering-magic-imports");


Should we use string-lowering-magic-imports-assert to make sure we aren't accidentally doing any optimizations that would result in us emitting a non-standard custom section for non-UTF-8 string constants?

Wait, isn't the section standardized?

No. The standard solution is the magic imports, which can only handle valid UTF-8 strings. The custom section is a random thing we experimented with on the way to developing magic imports.

What is the spec solution for non-UTF8 strings, then?

Separately, if we assert here, then any module with a non-utf8 stringref will assert if you just do -all -O2... that seems wrong to me.

There is no spec solution for non-UTF-8 strings. We could synthesize them in a start function if we had to, but I don't think that should be necessary. No input module should have non-UTF-8 strings because they are not expressible with string builtins, and our optimizations should not produce new invalid non-UTF-8 strings, so no output module should have non-UTF-8 strings. Input modules that use stringref to represent non-UTF-8 strings probably don't want this lowering to occur in the first place.

We can do the lifting/lowering only when stringref is not enabled

Hmm, seems weird for -all to do less than -all --disable-stringref. More features should mean more things to optimize, normally.

Ok, we can do the lifting when either stringref or string-builtins are enabled, then lower only if string-builtins and not stringref are enabled.

and also make non-UTF-8 string.const a validation error when stringref is not enabled.

When stringref is disabled, any string.const (UTF8 or otherwise) is invalid anyhow?

string.const should still be valid when string-builtins are enabled. We might want to error in binary writing if they haven't been lowered and stringref is not enabled, though.

Hmm, these are valid options, but they all seem significantly more complex to explain and to use. The readme text would need to be substantially longer.

I don't have a better suggestion for this PR yet, but let's keep thinking here.

On the contrary, I don't think there's much to explain. As always, we optimize to the extent we can given the enabled features, and we validate that the IR will be written as valid modules given the enabled features

We should document that string.const is valid IR if either of the string features are enabled, but only on the condition that it will be lowered before writing if stringref is disabled. We should also document that string.const containing unpaired surrogates is only valid if stringref is enabled. I don't think we need to document how automatic lifting and lowering works, just as we don't need to document the specifics of what other optimizations we run.

I'd say all of these quotes are non-trivial and potentially confusing for our users:

"do the lifting when either stringref or string-builtins are enabled, then lower only if string-builtins and not stringref are enabled"

"string.const is valid IR if either of the string features are enabled, but only on the condition that it will be lowered before writing if stringref is disabled"

"string.const containing unpaired surrogates is only valid if stringref is enabled."

Just adding that text by itself would double the current PR's readme entry.

I really feel we should find something simpler here.

I'd say all of these quotes are non-trivial and potentially confusing for our users:

"do the lifting when either stringref or string-builtins are enabled, then lower only if string-builtins and not stringref are enabled"

This is just us doing optimizations based on the target features. It doesn't need to be documented or understood by users.

"string.const is valid IR if either of the string features are enabled, but only on the condition that it will be lowered before writing if stringref is disabled"

Yeah, this one is weird. If lowering were automatic in the binary writer, this wouldn't need to be documented or understood, either. This complexity is the price we pay to make lowering a separately sequenced pass.

"string.const containing unpaired surrogates is only valid if stringref is enabled."

This one just matches the expressive capabilities of the underlying features. I don't think it needs to be documented beyond error messages.

Just adding that text by itself would double the current PR's readme entry.

I really feel we should find something simpler here.

This is the validation behavior we would want independent of how we lift and lower strings. The only reason this complexity comes up in this PR and not before is because we didn't have separate string features before this PR. But it's strictly more useful to users to have separate string features with detailed validation to ensure they actually produce valid binaries for the intended engine feature set.

kripken added 13 commits May 15, 2025 15:57

start

5db98f4

work

36d89d5

docs

956d45a

p

b72d753

format

3b21cf2

work

adad0dc

work

c636685

finish

7bf1cf7

Merge remote-tracking branch 'origin/main' into sb

8ad61b5

test

48f05f7

test

60637f9

test

3195ac2

test

4dd6747

kripken requested a review from tlively May 16, 2025 00:00

tlively approved these changes May 16, 2025

View reviewed changes

use magic imports [ci skip]

3c29e7b

kripken mentioned this pull request May 16, 2025

[Strings] Automate string lifting and lowering in the optimization pipeline #7540

Closed

tlively added a commit that referenced this pull request May 16, 2025

[NFC] Remove StringLowering::replaceNulls

4f9d5eb

Now that `string` is a subtype of `extern`, the null type for strings and externrefs is the same, so we no longer need to fix up nulls in StringLowering. Unblocks #7601.

tlively mentioned this pull request May 16, 2025

[NFC] Remove StringLowering::replaceNulls #7607

Merged

kripken added 7 commits May 16, 2025 12:00

Merge remote-tracking branch 'origin/main' into sb

8ae39b8

gate the pass behind GC

bbd6804

update test

2e82f9a

work

0feeeae

update exact result in test, likely because of the new pass we run

02d1d23

so many tests to update

57ac595

fix feature reading

00b3c1b

kripken added 4 commits May 19, 2025 07:40

fix unit test

123b879

gc is needed too, now, for string optimization

dfa5231

fix: some tests have type updates due to another pass being run

77b7f6e

simplify test as suggested

17c5019

tlively reviewed May 19, 2025

View reviewed changes

don't fuzz string builtins yet

0a447e5

tlively reviewed May 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Strings] Add a string-builtins feature, and lift/lower automatically when enabled #7601

[Strings] Add a string-builtins feature, and lift/lower automatically when enabled #7601

kripken commented May 16, 2025 •

edited

Loading

tlively left a comment

tlively May 16, 2025

kripken May 19, 2025

tlively May 16, 2025

kripken May 19, 2025

kripken commented May 16, 2025

tlively commented May 16, 2025

kripken commented May 16, 2025

tlively May 19, 2025

kripken May 19, 2025

tlively May 19, 2025

kripken May 19, 2025

tlively May 19, 2025

kripken May 19, 2025

tlively May 19, 2025

tlively May 19, 2025

kripken May 19, 2025

tlively May 19, 2025

kripken May 19, 2025

tlively May 19, 2025

[Strings] Add a string-builtins feature, and lift/lower automatically when enabled #7601

Are you sure you want to change the base?

[Strings] Add a string-builtins feature, and lift/lower automatically when enabled #7601

Conversation

kripken commented May 16, 2025 • edited Loading

tlively left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented May 16, 2025

tlively commented May 16, 2025

kripken commented May 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented May 16, 2025 •

edited

Loading