Skip to content
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.

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

[Wasm GC] Support non-nullable locals in the "1a" form #4959

Merged
merged 261 commits into from
Aug 31, 2022
Merged

[Wasm GC] Support non-nullable locals in the "1a" form #4959

merged 261 commits into from
Aug 31, 2022

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 23, 2022

An overview of this is in the README in the diff here (conveniently, it is near the
top of the diff). Basically, we fix up nn locals after each pass, by default. This keeps
things easy to reason about - what validates is what is valid wasm - but there are
some minor nuances as mentioned there, in particular, we ignore nameless blocks
(which are commonly added by various passes; ignoring them means we can keep
more locals non-nullable).

The key addition here is LocalStructuralDominance which checks which local
indexes have the "structural dominance" property of 1a, that is, that each get has
a set in its block or an outer block that precedes it. I optimized that function quite
a lot to reduce the overhead of running that logic after each pass. The overhead
is something like 2% on J2Wasm and 0% on Dart (0%, because in this mode we
shrink code size, so there is less work actually, and it balances out).

Since we run fixups after each pass, this PR removes logic to manually call the
fixup code from various places we used to call it (like eh-utils and various passes).

Various passes are now marked as requiresNonNullableLocalFixups => false.
That lets us skip running the fixups after them, which we normally do automatically.
This helps avoid overhead. Most passes still need the fixups, though - any pass
that adds a local, or a named block, or moves code around, likely does.

This removes a hack in SimplifyLocals that is no longer needed. Before we
worked to avoid moving a set into a try, as it might not validate. Now, we just do it
and let fixups happen automatically if they need to: in the common code they
probably don't, so the extra complexity seems not worth it.

Also removes a hack from StackIR. That hack tried to avoid roundtrip adding a
nondefaultable local. But we have the logic to fix that up now, and opts will
likely keep it non-nullable as well.

Various tests end up updated here because now a local can be non-nullable -
previous fixups are no longer needed.

Note that this doesn't remove the gc-nn-locals feature. That has been useful for
testing, and may still be useful in the future - it basically just allows nn locals in
all positions (that can't read the null default value at the entry). We can consider
removing it separately.

Fixes #4824

test/lit/non-nullable-locals.wast Outdated Show resolved Hide resolved
test/lit/passes/coalesce-locals-gc.wast Outdated Show resolved Hide resolved
test/lit/passes/heap2local.wast Outdated Show resolved Hide resolved
test/lit/passes/local-subtyping.wast Outdated Show resolved Hide resolved
test/lit/passes/roundtrip-gc.wast Show resolved Hide resolved
test/lit/passes/simplify-locals-gc-nn-1a.wast Outdated Show resolved Hide resolved
test/lit/validation/bad-non-nullable-locals.wast Outdated Show resolved Hide resolved
@@ -1,17 +0,0 @@
;; Test for non-nullable types in tuples
Copy link
Member

Choose a reason for hiding this comment

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

Why was this deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

This tested a case that now validates fine (non-nullable entries in tuples).

I guess it could be manually rewritten to show the opposite result, if we think that's important? (the auto-updater script can't do it for us here, and I'm not sure it's worth the work)

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to test that non-nullable tuples are ok now with the proper feature set enabled. If we only have one RUN line, this could be handled by the auto update script.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I restored the test and made it just check validation.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Excellent, LGTM!

@kripken kripken merged commit 8e1abd1 into main Aug 31, 2022
@kripken kripken deleted the 1a-fixups branch August 31, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-nullable locals, option 1a
2 participants