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

[EH] Add --experimental-new-eh option to wasm-opt #6270

Merged
merged 7 commits into from
Feb 6, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Feb 2, 2024

This adds --experimental-new-eh option to wasm-opt. The difference between this and --translate-to-new-eh is, --translate-to-new-eh just runs TranslateToNewEH pass, while --experimental-new-eh attaches TranslateToNewEH pass at the end of the whole optimization pipeline. So if no other passes or optimization options (-On) are specified, it is equivalent to --translate-to-new-eh. If other optimization passes are specified, it runs them and at the end run the translator to ensure the new EH instructions are emitted. The reason we are doing this this way is that the optimization pipeline as a whole does not support the new EH instruction yet, but we would like to provide an option to emit a reasonably OK code with the new EH instructions.

This also means when the optimization level > 3, it will also run the StackIR + local2stack optimization after the translation.

Not sure how to test the output of this option, given that there is not much point in testing the default optimization passes, and it is also not clear how to print the stack IR if the stack ir generation and optimization runs as a part of the pipeline and not the explicit command line options.

This is created in favor of #6267, which added the option to optimization-options.h. It had a problem of running the translator multiple times when -On was given multiple times in the command line, which I learned was rather a common usage. This adds the option directly to wasm-opt.cpp, which avoids the problem. With this, it is still possible to create and optimize Stack IR unnecessarily, but that feels a better alternative.

This adds `--experimental-new-eh` option to `wasm-opt`. The difference
between this and `--translate-to-new-eh` is, `--translate-to-new-eh`
just runs `TranslateToNewEH` pass, while `--experimental-new-eh`
attaches `TranslateToNewEH` pass at the end of the whole optimization
pipeline. So if no other passes or optimization options (`-On`) are
specified, it is equivalent to `--translate-to-new-eh`. If other
optimization passes are specified, it runs them and at the end run the
translator to ensure the new EH instructions are emitted. The reason we
are doing this this way is that the optimization pipeline as a whole
does not support the new EH instruction yet, but we would like to
provide an option to emit a reasonably OK code with the new EH
instructions.

This also means when the optimization level > 3, it will also run
the StackIR + local2stack optimization after the translation.

Not sure how to test the output of this option, given that there is not
much point in testing the default optimization passes, and it is also
not clear how to print the stack IR if the stack ir generation and
optimization runs as a part of the pipeline and not the explicit command
line options.

This is created in favor of WebAssembly#6267, which added the option to
`optimization-options.h`. This had a problem of running the translator
multiple times when `-On` was given multiple times in the command line,
which I learned was rather a common usage. This adds the option directly
to `wasm-opt.cpp`, which avoids the problem. With this, it is still
possible to create and optimize Stack IR unnecessarily, but that feels a
better alternative.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Code lgtm.

I think a test would be good to add just to be safe. Maybe we can check in the result of --experiment-new-eh -O on a binary, and a lit test could just check that the result has not changed? (just a binary diff with a boolean outcome)

src/tools/wasm-opt.cpp Outdated Show resolved Hide resolved
@aheejin
Copy link
Member Author

aheejin commented Feb 2, 2024

Code lgtm.

I think a test would be good to add just to be safe. Maybe we can check in the result of --experiment-new-eh -O on a binary, and a lit test could just check that the result has not changed? (just a binary diff with a boolean outcome)

Not sure if I understand. What binary? (Old EH instructions or new ones?) Why does it have to be unchanged? And why would it be a binary test rather than a text one?

I can add a test showing that --experimental-new-eh does the same thing (translation) w/ --translate-to-new-eh when given alone. It is hard to test the combination of the translation and stack IR optimization because there is no way I can print the stack IR result with this.

@kripken
Copy link
Member

kripken commented Feb 2, 2024

I mean taking a file old.wasm with old wasm EH, running wasm-opt --experimental-new-eh -O -o new.wasm, and checking in both old.wasm and new.wasm. The test would run that same command and see that it is the expected binary.

If we had wabt available then we could diff the text of the output, but without that, I think we can just compare the binaries as a boolean. That is, in this PR we can manually verify with wabt or another tool that the binary is properly optimized, including stackIR opts have run. Then the test will make sure we don't regress on that test.

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.

Instead of checking in binaries for the tests, could we have the lit test assemble input.wat using both --experimental-new-eh and a manual pipeline using --translate-to-new-eh and check that the results are the same?

Other than that, LGTM.

@aheejin
Copy link
Member Author

aheejin commented Feb 2, 2024

Instead of checking in binaries for the tests, could we have the lit test assemble input.wat using both --experimental-new-eh and a manual pipeline using --translate-to-new-eh and check that the results are the same?

Do you mean

wasm-opt --experimental-new-eh [INPUT] -o [OUTPUT]

and

wasm-opt --translate-to-new-eh [INPUT] -o [OUTPUT]

produce the same output binary (from the given input wat file), when each of these is given as a standalone flag?

@tlively
Copy link
Member

tlively commented Feb 2, 2024

Yes, exactly. And maybe you could add the -O1 and --generate-stack-ir --optimize-stack-ir flags to get them to run the same optimization pipeline as well.

@aheejin
Copy link
Member Author

aheejin commented Feb 6, 2024

@tlively PTAL 😀

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.

LGTM with that comment, unless we can't get rid of the intermediate file, in which case LGTM as-is.

Comment on lines 10 to 11
;; RUN: wasm-opt %s -all -O --translate-to-new-eh -o %t.wasm
;; RUN: wasm-opt %t.wasm -all --optimize-level=3 --generate-stack-ir --optimize-stack-ir -o %a.wasm
Copy link
Member

Choose a reason for hiding this comment

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

Can we do without the intermediate file here? (@kripken, I don't quite remember how the optimization-level and -O flags interact these days)

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 think we can do without it... Not sure why I thought otherwise. Will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the intermediate file. Will merge this. Thanks!

@aheejin aheejin merged commit 3db60df into WebAssembly:main Feb 6, 2024
14 checks passed
@aheejin aheejin deleted the new_eh_option2 branch February 6, 2024 20:22
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
This adds `--experimental-new-eh` option to `wasm-opt`. The difference
between this and `--translate-to-new-eh` is, `--translate-to-new-eh`
just runs `TranslateToNewEH` pass, while `--experimental-new-eh`
attaches `TranslateToNewEH` pass at the end of the whole optimization
pipeline. So if no other passes or optimization options (`-On`) are
specified, it is equivalent to `--translate-to-new-eh`. If other
optimization passes are specified, it runs them and at the end run the
translator to ensure the new EH instructions are emitted. The reason we
are doing this this way is that the optimization pipeline as a whole
does not support the new EH instruction yet, but we would like to
provide an option to emit a reasonably OK code with the new EH
instructions.

This also means when the optimization level > 3, it will also run
the StackIR + local2stack optimization after the translation.

Not sure how to test the output of this option, given that there is not
much point in testing the default optimization passes, and it is also
not clear how to print the stack IR if the stack ir generation and
optimization runs as a part of the pipeline and not the explicit command
line options.

This is created in favor of WebAssembly#6267, which added the option to
`optimization-options.h`. It had a problem of running the translator
multiple times when `-On` was given multiple times in the command line,
which I learned was rather a common usage. This adds the option directly
to `wasm-opt.cpp`, which avoids the problem. With this, it is still
possible to create and optimize Stack IR unnecessarily, but that feels a
better alternative.
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

3 participants