Skip to content

Conversation

@vouillon
Copy link
Contributor

@vouillon vouillon commented Apr 8, 2024

Use the previous implementation when no return_call is in a try block. This avoids moving code around (as a sibling of the caller body or the inlined body), so that should allow more local optimizations after inlining.

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.

Thanks, this LGTM! Have you had a chance to run the fuzzer on this? Usually running it for about 10k iterations is enough to give us confidence for a change like this.

@vouillon
Copy link
Contributor Author

vouillon commented Apr 8, 2024

It seems the fuzzer does not like the last commit. I think that's because try_table is not fully supported yet. For instance, fuzz_opt.py 9274061891157013786 yields the following error:

unimplemented DCE control flow structure
UNREACHABLE executed at /home/jerome/sources/binaryen/src/passes/DeadCodeElimination.cpp:189!

Without this commit, I have been able to run it for about 10k iterations, with only a few failures due to try_table, but maybe I'm no longer testing tail calls within try blocks?

@tlively
Copy link
Member

tlively commented Apr 8, 2024

Ahh, right. I was hoping to use try_table since that's the new standard instruction, but we should change the tests to use try instead because that's better supported by all the optimization passes. The fuzzer shouldn't have any issues with try.

@vouillon vouillon force-pushed the return-call-inlining branch from f2c2a16 to 2e62ffe Compare April 9, 2024 14:59
@vouillon
Copy link
Contributor Author

I did not found any issue with the fuzzer.

@tlively
Copy link
Member

tlively commented Apr 10, 2024

Great, thanks!

@tlively tlively merged commit f7ea0c4 into WebAssembly:main Apr 10, 2024
@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.

2 participants