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

[RFC] Remove the old x86 backend #3009

Merged
merged 14 commits into from
Sep 30, 2021
Merged

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Jun 21, 2021

The default has been switched to the new x64 backend a while ago. AFAIK nobody has had any problems with the new x64 backend that required switching back to the old x86 backend.

This PR removes the old x86 backend, and the cranelift-codegen-meta part of the encoding and legalization mechanism of the old backend. The cranelift-codegen parts are only removed where necessary to fix warnings.

Based on #3007

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 21, 2021

The default was changed only two and a half months ago (#2718). It felt much longer.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels Jun 21, 2021
@cfallin
Copy link
Member

cfallin commented Jun 21, 2021

Thanks so much for tackling this! It has been on my to-do list for a while, but at low priority relative to a bunch of other things, so I'm happy that you had time to pick it up :-)

I think we should probably write up a proper RFC in bytecodealliance/rfcs, just to ensure we have the right folks onboard and there are no remaining surprise uses of the backend. I'll draft something soon and then we can start discussion.

cfallin added a commit to cfallin/wasmtime that referenced this pull request Jun 24, 2021
These backends will be removed in the future (see
bytecodealliance/rfcs#12 and the pending bytecodealliance#3009 in this repo).

In the meantime, to more clearly communicate that they are using
"legacy" APIs and will eventually be removed, this PR places them in an
`isa/legacy/` subdirectory. No functional changes otherwise.
@cfallin
Copy link
Member

cfallin commented Sep 28, 2021

@bjorn3 now that the RFC is merged, would you be interested in bringing this PR up-to-date? If not, no worries, someone else can pick up the effort, but please do feel free if you're up for it, and I'm happy to review.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 28, 2021

I will try to update it tomorrow.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 29, 2021

@cfallin Rebased

@cfallin
Copy link
Member

cfallin commented Sep 29, 2021

@bjorn3 there's a build failure that seems to arise from some missing DSL bits -- perhaps cut just a bit too much?

(As an aside, I'm very happy to see the -40k LoC stat; that's a lot of code we won't have to maintain!)

@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 29, 2021

I forgot to remove two tests it seems. They were testing parts I cut out.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 29, 2021

(As an aside, I'm very happy to see the -40k LoC stat; that's a lot of code we won't have to maintain!)

~16k of deletions is tests for the old x86 backend or the old backend infrastructure. ~24k deletions are for cranelift-codegen.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 29, 2021

FAIL filetests/filetests/runtests/extend.clif: run

Caused by:
    0: Cranelift codegen error
    1: Unsupported feature: Isplit: Unsupported type: types::I64

How did this test work on AArch64 in the first place? isplit from i64 -> i32, i32 shouldn't have been implemented on any new backend at all.

Should I remove it?

@cfallin
Copy link
Member

cfallin commented Sep 29, 2021

FAIL filetests/filetests/runtests/extend.clif: run

Caused by:
    0: Cranelift codegen error
    1: Unsupported feature: Isplit: Unsupported type: types::I64

How did this test work on AArch64 in the first place? isplit from i64 -> i32, i32 shouldn't have been implemented on any new backend at all.

Should I remove it?

Not sure, but for that particular test and its twin below, we could probably rewrite it without the isplit (maybe this is what you mean by "remove it") -- just a single 64-bit comparison rather than a split/two 32-bit compares/boolean-and.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 29, 2021

Rewrote it to remove the usage of isplit.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Major thanks for this -- it was a large amount of work!

I went back and forth a bit about removing BackendVariant -- the plumbing exists now, what if we need it -- but on balance, I think the right choice is what's in this PR now, namely, providing a single consistent API and going back to the notion that there is just one backend per architecture. If in the future we have a really good reason to again duplicate backends then we can add plumbing again.

Also, I didn't look at the code that has not been removed in detail to work out what else could be removed, but I think there's still a lot: for example, any mention of Encodings should eventually go away. But we can much more easily do that once the bulk of the old backend and associated infrastructure are removed; multiple followup PRs are expected and reasonable here, I think.

Anyway, LGTM and thanks again!

cranelift/codegen/src/isa/mod.rs Outdated Show resolved Hide resolved
@bjorn3 bjorn3 force-pushed the bye_x86_backend branch 2 times, most recently from 83a92c7 to 4b6d20d Compare September 29, 2021 20:08
@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 30, 2021

CI passes

@cfallin
Copy link
Member

cfallin commented Sep 30, 2021

Everything looks good -- well, here goes something! The old backend served us well, and thanks to those who worked on it; onward...

@dvc94ch
Copy link
Contributor

dvc94ch commented Nov 12, 2021

I guess x86/riscv support is canned for the moment? as in there aren't enough users for these targets?

@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 12, 2021

x86 is not a priority AFAIK, but it would be nice to have. Tracking issue: #1980

Riscv support has never been usable AFAICT. I believe it got prototyped with the original creation of Cranelift, but has never seen any improvements afterwards. Tracking issue: #2217

Nobody is currently working on either target to the best of my knowledge, but I am sure PR's for either will be accepted. @cfallin estimated that it will be 2-3 months of fulltime work to get to a reasonable state in #2217 (comment). If you do want to implement a backend, be aware of bytecodealliance/rfcs#15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

3 participants