Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
5db98f4
start
kripken May 15, 2025
36d89d5
work
kripken May 15, 2025
956d45a
docs
kripken May 15, 2025
b72d753
p
kripken May 15, 2025
3b21cf2
format
kripken May 15, 2025
adad0dc
work
kripken May 15, 2025
c636685
work
kripken May 15, 2025
7bf1cf7
finish
kripken May 15, 2025
8ad61b5
Merge remote-tracking branch 'origin/main' into sb
kripken May 15, 2025
48f05f7
test
kripken May 15, 2025
60637f9
test
kripken May 15, 2025
3195ac2
test
kripken May 15, 2025
4dd6747
test
kripken May 16, 2025
3c29e7b
use magic imports [ci skip]
kripken May 16, 2025
8ae39b8
Merge remote-tracking branch 'origin/main' into sb
kripken May 16, 2025
bbd6804
gate the pass behind GC
kripken May 16, 2025
2e82f9a
update test
kripken May 16, 2025
0feeeae
work
kripken May 16, 2025
02d1d23
update exact result in test, likely because of the new pass we run
kripken May 16, 2025
57ac595
so many tests to update
kripken May 16, 2025
00b3c1b
fix feature reading
kripken May 19, 2025
123b879
fix unit test
kripken May 19, 2025
dfa5231
gc is needed too, now, for string optimization
kripken May 19, 2025
77b7f6e
fix: some tests have type updates due to another pass being run
kripken May 19, 2025
17c5019
simplify test as suggested
kripken May 19, 2025
0a447e5
don't fuzz string builtins yet
kripken May 19, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
gate the pass behind GC
  • Loading branch information
@kripken
kripken committed May 16, 2025
commit bbd680434c14e592e56fc3b9b4cd2dead2eca8f3
16 changes: 12 additions & 4 deletions src/passes/pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,8 +727,11 @@ void PassRunner::addDefaultGlobalOptimizationPrePasses(Ordering ordering) {
// If we are optimizing string builtins then we lift at the very start of the
// optimization pipeline, not just at the beginning here, but only when we are
// ordered before other bundles of passes.
if (wasm->features.hasStringBuiltins() && options.optimizeLevel >= 2 &&
ordering.first) {
//
// We check for GC for symmetry with the lowering pass, see comment in
// addDefaultGlobalOptimizationPostPasses() below.
if (wasm->features.hasStringBuiltins() && wasm->features.hasGC() &&
options.optimizeLevel >= 2 && ordering.first) {
addIfNoDWARFIssues("string-lifting");
}
// Removing duplicate functions is fast and saves work later.
Expand Down Expand Up @@ -805,8 +808,13 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses(Ordering ordering) {
// Lower away strings at the very very end. We do this before
// remove-unused-module-elements so we don't add unused imports, and also
// before reorder-globals, which will sort the new globals.
if (wasm->features.hasStringBuiltins() && options.optimizeLevel >= 2 &&
ordering.last) {
//
// Note we also test for GC here, as the pass adds imports that use GC arrays
// (and externref). Those imports may be unused, but they exist until
// remove-unused-module-elements cleans them up, which would cause an error in
// between.
if (wasm->features.hasStringBuiltins() && wasm->features.hasGC() &&
options.optimizeLevel >= 2 && ordering.last) {
addIfNoDWARFIssues("string-lowering-magic-imports");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, isn't the section standardized?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
}

Expand Down