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

Wasmtime: run all spec tests with Cranelift-with-egraphs. #5062

Closed
wants to merge 1 commit into from

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Oct 17, 2022

This change adds another variant of the spec-tests run as part of the wasmtime-cli crate's unit tests: Cranelift with egraph-based optimizations enabled.

We intend to test egraphs in various ways before turning the mechanism on by default. We will certainly fuzz differentially, but it is also useful to run our basic testsuite against the new compiler mid-end as part of normal CI. There is already precedent for running the spec tests with a variant of settings (the _pooling versions of each test). The actual spec testsuite execution does not contribute appreciably to CI time compared to the time to compile Wasmtime itself, so running an extra variant of the tests should not increase test cost significantly.

This change adds another variant of the spec-tests run as part of the
`wasmtime-cli` crate's unit tests: Cranelift with egraph-based
optimizations enabled.

We intend to test egraphs in various ways before turning the mechanism
on by default. We will certainly fuzz differentially, but it is also
useful to run our basic testsuite against the new compiler mid-end
as part of normal CI. There is already precedent for running the spec
tests with a variant of settings (the `_pooling` versions of each test).
The actual spec testsuite execution does not contribute appreciably to
CI time compared to the time to compile Wasmtime itself, so running an
extra variant of the tests should not increase test cost significantly.
@cfallin cfallin requested review from elliottt and fitzgen October 17, 2022 06:13
@alexcrichton
Copy link
Member

Personally I've found that historically the spec test suite isn't really all that great at exercising compiler internals, so I'd expect to get a lot more bang-for-our-buck by updating fuzzing. Thinking again on this we should probably drop the *_pooling tests given the existence of the spectests fuzzer we have to simplify the testing setup (but not here in this PR of course).

Is the egraph implementation ready for overall enabling when fuzzing? If not perhaps this option could be added to fuzzing but only enabled for the spectests fuzzer?

@cfallin
Copy link
Member Author

cfallin commented Oct 17, 2022

Yeah, actually, that's totally fine with me too. Once #5020 lands we'll fuzz with-egraphs against no-egraphs so we'll have full coverage; I was thinking it might be nice to have a stopgap but that's close enough and the spectests are small enough that it's probably not worth it.

@cfallin cfallin closed this Oct 17, 2022
@alexcrichton
Copy link
Member

It looks like #5020 is only changing the cranelift-fuzzgen target, but would it also make sense to add this for Wasmtime's fuzzing?

@cfallin
Copy link
Member Author

cfallin commented Oct 17, 2022

Yes, actually, we should do that too; I'll follow up with a PR.

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