-
Notifications
You must be signed in to change notification settings - Fork 814
Flatten: Support br_on_null
, br_on_non_null
, br_on_cast
, br_on_cast_fail
#7939
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
base: main
Are you sure you want to change the base?
Conversation
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. | ||
;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. | ||
|
||
;; RUN: foreach %s %t wasm-opt --flatten --all-features -S -o - | filecheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The foreach
here lets you have multiple modules in the same test file, but since the pass just does function-local changes, you can even just have a single module with several functions in it. This would be more in line with how we usually group tests together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberately created different files as I found it to be easier to modify the tests this way.
I also get more detailed error reporting, if I have a bug in one of the instructions but others work fine, 3 out of 4 tests pass (instead of a large file with lots of instructions failing with just one error).
I can still merge these to one of the existing files if you prefer. Which file should I move these to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess with multiple modules I would get multiple errors in the same file, right? Instead of just one error for all of the tests in the file.
It's still easier to edit multiple files, but if you let me know where you want these tests I can move them to another file. There's a flatten_all-features.wast
, but it's kinda large, and it mixes underscores and dashes in the same file name 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, with multiple modules you can get multiple errors. Keeping the separate modules is fine, but I would consolidate all the tests in a single flatten-br-on.wast
file. (The mixed underscores and dashes in flatten_all-features.wast
is baggage from the older test system that parsed command line options from file names 🫣)
@tlively @kripken This is ready for reviews, but the testing here may not be sufficient. Because I use I'm not sure how to test for this as we would need to run the lowered program. For now I tested this on a few dart2wasm benchmarks (largest one being a 37M .wasm) and it works fine. Any suggestions on how to improve testing here? I suggest reviewing the transformations shown in the PR description first. If those transformations are not right then the code also won't be right. (unless I made a mistake while transcribing) |
Fixes #6989.
With this we're able to use
-O4
to optimize dart2wasm programs. I tried this on a few benchmarks, largest one being a 37M .wasm, and it worked fine.Flattening transformations done: