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

feat(compiler): Cleaner wasm output for low-level wasm types #1158

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

ospencer
Copy link
Member

@ospencer ospencer commented Mar 6, 2022

This avoids an issue in Binaryen v102 where spurious multivalue types appear because of our use of tuples (by using far fewer tuples).

@ospencer ospencer requested a review from a team March 6, 2022 21:25
@ospencer ospencer self-assigned this Mar 6, 2022
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Amazing work 🙏 I think we have 2 follow-ups to this:

  1. Please create an issue to add a NEAR smoketest to our CI - you can assign it to me, but it'll block dropping Node 14 support
  2. We need to document, or file an issue, or write a fix for binaryen. I'm not sure the best way to track this.

@ospencer
Copy link
Member Author

ospencer commented Mar 7, 2022

Smoketest issue is #1160. I'm not really sure how to track the binaryen issue either. Maybe a promise that I'll do it? 😛

I'm really still just trying to provide a good repro for them with binaryen.ml—I have a small Grain program that repros, but I would feel bad submitting that to them.

@ospencer
Copy link
Member Author

ospencer commented Mar 8, 2022

Excellent news (in a way, we may have to abandon our stack-hack with tuples)—there's no Binaryen bug at all. They added an optimization that lifts identical code from the arms of ifs and selects, so if you have something like if (...) then tuple_extract(tuple_make(...)) else tuple_extract(tuple_make(...)) that'll get optimized to tuple_extract(if (...) then tuple_make(...) else tuple_make(...), which introduces multivalue types on the if itself. I suppose it gets you slightly smaller wasm output in our case since you'd only need one drop instead of two.

@ospencer
Copy link
Member Author

ospencer commented Mar 8, 2022

Called this out over in WebAssembly/binaryen#4526

@ospencer ospencer force-pushed the oscar/fewer-tuples branch 2 times, most recently from 1692a4f to 44ddf1e Compare March 18, 2022 20:09
Copy link
Member

@peblair peblair left a comment

Choose a reason for hiding this comment

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

(a,b)[0] 🙅‍♂️

@ospencer ospencer merged commit 88060dd into main Apr 1, 2022
@ospencer ospencer deleted the oscar/fewer-tuples branch April 1, 2022 00:12
@github-actions github-actions bot mentioned this pull request May 16, 2022
@github-actions github-actions bot mentioned this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants