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

cranelift: arm32 codegen #2103

Merged
merged 2 commits into from
Sep 22, 2020
Merged

cranelift: arm32 codegen #2103

merged 2 commits into from
Sep 22, 2020

Conversation

jmkrauz
Copy link
Contributor

@jmkrauz jmkrauz commented Aug 6, 2020

This commit adds arm32 Thumb2 code generation for some IR insts.
I have used machinst backend and followed Aarch64 implementation as an example.

Floating-point instructions are not supported, because regalloc
does not seem to allow to represent overlapping register classes,
which are needed by VFP/Neon.

Another lacking feature is support for I64 and I128 types.

An abi implementation is not complete, however it makes it possible
to run simple clif tests.

It may be a first step towards #1173.

This PR contains instruction emission test and clif tests for IR insts.

I don't know well who could review this.

}
// Arm32 `TargetIsa` is now `TargetIsaAdapter`, which does not hold any info
// about registers, so we directly access `INFO` from registers-arm32.rs.
include!(concat!(env!("OUT_DIR"), "/registers-arm32.rs"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use x86 or riscv instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe some of the tests here or in regalloc/pressure.rs use the fact that S, D and Q registers overlap. I am not sure if we can achieve something similar with x86 or risc-v registers.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Aug 6, 2020
@github-actions
Copy link

github-actions bot commented Aug 6, 2020

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@cfallin
Copy link
Member

cfallin commented Aug 6, 2020

Wow -- this is an excellent surprise to start my day with. First of all, thanks, @jmkrauz, for putting in all of this work!

I'll review this in the next few days; I expect it will take a bit of time. One high-level comment regarding the register-overlap issue: we've thought about (and need to solve) it for other architectures too, e.g. x86-32, so I expect we'll have a better answer eventually. For now, a stopgap solution (for correctness) might be to simply allocate pairs as the fundamental unit (i.e. D regs), and just burn an extra 32-bit register for each f32, and punt on 128-bit vector support for now. Not ideal but it will get us to basic completeness at least. @julian-seward1 might have more thoughts on this?

@cfallin cfallin self-requested a review August 6, 2020 16:18
@cfallin
Copy link
Member

cfallin commented Aug 21, 2020

@jmkrauz, I just wanted to comment here to say first of all, we haven't forgotten this -- we're working on some framework improvements that should make this backend a little bit nicer. Specifically:

  • In recent PRs I've refactored the ABI code to provide a higher-level abstraction when an ABI follows normal-ish conventions; this reduces a lot of the duplicated code between aarch64 and x64, at least. Would you be able to refactor the ABI support in this patch to use that framework (ABIMachineImpl trait etc)? There are a few places where it assumes 64-bitness but we can work through fixing those easily enough...

  • I'm currently working out isel framework improvements to allow one SSA value to live in multiple registers; this will permit us to support i64 on 32-bit targets (and later i128 everywhere). I think we probably want i64 support before this is merged, but we can see how that goes as the framework comes together.

I also don't want to hold this in limbo forever; at some point (@julian-seward1 and @bnjbvr may have opinions as well) we can consider merging and gating behind an experimental feature flag until we get this up to Wasm-MVP (i32/i64/f32/f64 support) level.

Thanks again and hopefully this will come together sooner rather than later :-)

@jmkrauz
Copy link
Contributor Author

jmkrauz commented Aug 22, 2020

That's good news :)
I will work on this after the weekend.

@bnjbvr
Copy link
Member

bnjbvr commented Aug 24, 2020

Agreed that we could review and merge this behind a flag, or just letting know that support for arm32 is experimental; we've done this for other backends, and we can work incrementally on supporting more opcodes etc.

@github-actions github-actions bot added cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen labels Sep 16, 2020
@cfallin
Copy link
Member

cfallin commented Sep 22, 2020

Hi @jmkrauz, it looks like you've refactored to use the common abi_impl machinery (thanks!). I discussed this PR briefly today with a few others and we agree that it might be best to merge now, given that I haven't resolved the multiple-register-value question yet (which this would need for i64 values). (Sorry about that -- a lot on my plate lately!) If that's alright with you, of course; does this seem OK or do you have plans to refine the PR more first?

One thing I think we should do, however, is to put the new backend behind a feature flag until it implements Wasm MVP; otherwise someone might try to build wasmtime on arm32 and hit confusing panics. Perhaps just #[cfg(feature = "arm32")] on the main mod arm32 statement in cranelift/codegen/src/isa/mod.rs?

Jakub Krauz added 2 commits September 22, 2020 12:49
This commit adds arm32 code generation for some IR insts.
Floating-point instructions are not supported, because regalloc
does not allow to represent overlapping register classes,
which are needed by VFP/Neon.

There is also no support for big-endianness, I64 and I128 types.
@jmkrauz
Copy link
Contributor Author

jmkrauz commented Sep 22, 2020

Merging now is absolutely fine with me.
I have added additional commit in which I put ARM backend behind experimental_arm32 feature flag. In order to avoid failing arm32 filetests with experimental_arm32 disabled, I have also added small check in cranelift/filetests/src/runone.rs.

@cfallin
Copy link
Member

cfallin commented Sep 22, 2020

Excellent, thanks! Looking forward to seeing this become a full backend!

@cfallin cfallin merged commit f754724 into bytecodealliance:main Sep 22, 2020
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 Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants