Stream: git-wasmtime

Topic: wasmtime / Issue #2103 cranelift: arm32 codegen

Wasmtime GitHub notifications bot (Aug 06 2020 at 10:22):

github-actions[bot] commented on Issue #2103:

Subscribe to Label Action

cc @bnjbvr

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 06 2020 at 16:17):

cfallin commented on Issue #2103:

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?

Wasmtime GitHub notifications bot (Aug 21 2020 at 16:38):

cfallin commented on Issue #2103:

@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 :-)

Wasmtime GitHub notifications bot (Aug 22 2020 at 06:36):

jmkrauz commented on Issue #2103:

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

Wasmtime GitHub notifications bot (Aug 24 2020 at 08:50):

bnjbvr commented on Issue #2103:

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.

Wasmtime GitHub notifications bot (Sep 22 2020 at 00:43):

cfallin commented on Issue #2103:

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?

Wasmtime GitHub notifications bot (Sep 22 2020 at 11:17):

jmkrauz commented on Issue #2103:

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.

Wasmtime GitHub notifications bot (Sep 22 2020 at 16:07):

cfallin commented on Issue #2103:

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

Last updated: Nov 22 2024 at 16:03 UTC