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

Don't run old x86 backend-specific tests with new x64 backend. #2411

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Nov 13, 2020

Some of the test failures tracked by #2079 are in unwind tests that are
specific to the old x86 backend: namely, these tests invoke the unwind
implementation that is paired with the old backend, rather than generic
over all backends. It thus doesn't make sense to try to run these tests
with the new backend. (The new backend's unwind code should have
analogous tests written/ported over eventually.)

It seems that we were actually building both x86 backends when the
x64 feature was enabled, except that the old x86 backend would never
be instantiated by the usual ISA-lookup logic because a x86-64 target
triple unconditionally resolves to the new one.

This PR resolves both of the issues by tweaking the feature-config
directives to exclude the x86 backend when x64 is enabled.

Some of the test failures tracked by bytecodealliance#2079 are in unwind tests that are
specific to the old x86 backend: namely, these tests invoke the unwind
implementation that is paired with the old backend, rather than generic
over all backends. It thus doesn't make sense to try to run these tests
with the new backend. (The new backend's unwind code should have
analogous tests written/ported over eventually.)

It seems that we were actually building *both* x86 backends when the
`x64` feature was enabled, except that the old x86 backend would never
be instantiated by the usual ISA-lookup logic because a `x86-64` target
triple unconditionally resolves to the new one.

This PR resolves both of the issues by tweaking the feature-config
directives to exclude the `x86` backend when `x64` is enabled.
@cfallin cfallin added the cranelift:area:x86 Issues related to x86 codegen label Nov 13, 2020
@cfallin cfallin requested a review from abrown November 13, 2020 04:49
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Nov 13, 2020
@tschneidereit
Copy link
Member

(The new backend's unwind code should have
analogous tests written/ported over eventually.)

Would it make sense to file an issue for this so it doesn't get lost?

Copy link
Contributor

@abrown abrown left a comment

Choose a reason for hiding this comment

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

Makes sense to me. When we remove the old backend, can those tests be ported to the x64 backend? Or I guess I mean, should they be?

@cfallin
Copy link
Member Author

cfallin commented Nov 13, 2020

Indeed (to both), we will need to port the tests over before the old backend goes away! Created #2415 for this.

@cfallin cfallin merged commit 88fce76 into bytecodealliance:main Nov 13, 2020
@cfallin cfallin deleted the x86-backend-cfg branch January 6, 2021 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:x86 Issues related to x86 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants