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

Switch default to new x86_64 backend. #2718

Merged
merged 1 commit into from
Apr 5, 2021

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Mar 10, 2021

This PR switches the default backend on x86, for both the
cranelift-codegen crate and for Wasmtime, to the new
(MachInst-style, VCode-based) backend that has been under
development and testing for some time now.

The old backend is still available by default in builds with the
old-x86-backend feature, or by requesting BackendVariant::Legacy
from the appropriate APIs.

As part of that switch, it adds some more runtime-configurable plumbing
to the testing infrastructure so that tests can be run using the
appropriate backend. clif-util test is now capable of parsing a
backend selector option from filetests and instantiating the correct
backend.

I has been updated so that the old x86 backend continues to run its
tests, just as we used to run the new x64 backend separately.

At some point, we will remove the old x86 backend entirely, once we are
satisfied that the new backend has not caused any unforeseen issues and
we do not need to revert.

This implements the switch described in bytecodealliance/rfcs#10; we should
not merge this until we reach consensus on the process described therein
and merge the RFC.

@cfallin
Copy link
Member Author

cfallin commented Mar 10, 2021

Switching to "Ready to Review" to get a CI run, but this should still not be merged yet.

@cfallin cfallin marked this pull request as ready for review March 10, 2021 08:02
crates/fuzzing/Cargo.toml Outdated Show resolved Hide resolved
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. labels Mar 10, 2021
@cfallin
Copy link
Member Author

cfallin commented Mar 10, 2021

Hmm -- I'm not able to reproduce the segfault with the beta toolchain; I've tested on my usual Fedora 33 and then also Ubuntu 18.04 as GitHub uses. I'm wondering if it may be either a CPU-feature issue or a heisenbug -- will continue to probe at this.

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 10, 2021

I wonder if it would be possible to get GHA to generate a core dump that could be uploaded as artifact.

@cfallin cfallin force-pushed the new-backend branch 7 times, most recently from 153cca2 to f32f60d Compare March 11, 2021 02:48
@cfallin cfallin force-pushed the new-backend branch 3 times, most recently from a426da7 to 184a039 Compare March 12, 2021 04:57
@github-actions github-actions bot added the cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. label Mar 12, 2021
@cfallin
Copy link
Member Author

cfallin commented Mar 26, 2021 via email

@cfallin
Copy link
Member Author

cfallin commented Mar 30, 2021

Rebased to latest. The current plan described in the recently merged bytecodealliance/rfcs#10 is to:

  1. Wait until next week as the oss-fuzz fuzzers run with the new backend enabled (turned on in Update wasmtime config to use new x86-64 backend. google/oss-fuzz#5518).
  2. Make a final release of wasmtime/Cranelift with the current (old) default backend.
  3. Review and merge this PR.

@alexcrichton
Copy link
Member

FWIW the fuzzers are definitely working because they already fixed a fuzz bug that was about a timeout in the old backend. Our of curiosity is there any particular reason to wait for one more release rather than considering the last release of wasmtime the last release with the old backend as the default?

@cfallin
Copy link
Member Author

cfallin commented Mar 31, 2021

That's a good question -- the intent in the RFC was to have a latest-possible snapshot with as many features as possible supported on the old backend with the old backend on by default. Looking through the PR history since 0.25.0, though, it doesn't look like we landed any major user-visible feature, except for some things (SIMD tidbits on new x64 backend, macOS/aarch64) that are not relevant if one is using the old backend. (Please do let me know if I've missed something!)

Given that, I think it fits the spirit of the transition plan in the RFC to take 0.25.0 as the last-good old-backend-as-default release, and just merge this after a bit more fuzzing time. Curious what others think, of course!

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Sounds good to me! FWIW this looks good code-wise to me as well

cranelift/codegen/src/isa/x64/inst/args.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/x64/lower.rs Outdated Show resolved Hide resolved
This PR switches the default backend on x86, for both the
`cranelift-codegen` crate and for Wasmtime, to the new
(`MachInst`-style, `VCode`-based) backend that has been under
development and testing for some time now.

The old backend is still available by default in builds with the
`old-x86-backend` feature, or by requesting `BackendVariant::Legacy`
from the appropriate APIs.

As part of that switch, it adds some more runtime-configurable plumbing
to the testing infrastructure so that tests can be run using the
appropriate backend. `clif-util test` is now capable of parsing a
backend selector option from filetests and instantiating the correct
backend.

CI has been updated so that the old x86 backend continues to run its
tests, just as we used to run the new x64 backend separately.

At some point, we will remove the old x86 backend entirely, once we are
satisfied that the new backend has not caused any unforeseen issues and
we do not need to revert.
@cfallin
Copy link
Member Author

cfallin commented Apr 5, 2021

So there have been no fuzzbugs related to the new backend since we switched the fuzzers over; as per the above, I think this means we're ready to switch the default! Merging now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants