-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
The default was changed only two and a half months ago (#2718). It felt much longer. |
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. |
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.
@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. |
I will try to update it tomorrow. |
4cf9fab
to
53ec12d
Compare
@cfallin Rebased |
@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!) |
I forgot to remove two tests it seems. They were testing parts I cut out. |
~16k of deletions is tests for the old x86 backend or the old backend infrastructure. ~24k deletions are for cranelift-codegen. |
How did this test work on AArch64 in the first place? Should I remove it? |
Not sure, but for that particular test and its twin below, we could probably rewrite it without the |
Rewrote it to remove the usage of isplit. |
There was a problem hiding this 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 Encoding
s 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!
83a92c7
to
4b6d20d
Compare
fd5390c
to
463a88e
Compare
CI passes |
Everything looks good -- well, here goes something! The old backend served us well, and thanks to those who worked on it; onward... |
I guess x86/riscv support is canned for the moment? as in there aren't enough users for these targets? |
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. |
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