aarch64: emit SP copies when SP is involved in an explicit add during address lowering

[bnjbvr]

and use explicit address lowering for passing stack arguments to calls, when the offset is large.

One question is: what Inst should cause a move from SP to another register? I preferred to keep the "high-level" semantics of Inst::Mov, instead of relying on every producer to produce an Inst::Add { rd, sp, 0 }. This means we have to modify the codegen of Inst::Mov when SP is involved, but it seemed like fine blackboxing.

The PR as a whole is a bit messy. I was wondering if gen_copy_reg_to_arg shouldn't just take the ctx argument, but then type parameters were involved, making the trait more contrived. I didn't look into it, it might be the right thing to do in the future (otherwise this aarch64 impl detail leaks into the general interface, which is bad).

Good thing is that thanks to many new assertions, error like this one should be more easily caught in the future.

cc @julian-seward1 because I was hitting an assertion in regalloc if I didn't include the change in is_move: assert!(iix_bounds.uses_len == 1); (backtracking.rs:572). That happens during coalescing; I checked and the value was 0, instead of the expected 1. I don't think the change in is_move ought to be correct, so it'd be nice to see if there's something else to do to fix this.

[github-actions]

Subscribe to Label Action

cc @bnjbvr

Details

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64"

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]

Thanks for this! As mentioned below, I'm happy to clean up the design a bit to not require these temporaries afterward; don't want to block our correctness push on account of it, though. Good to go after a small comment tweak below.

[cfallin]

Hmm. I don't want to block this PR on it, but I'm thinking we should really clean up the ABI traits to take a LowerCtx and just emit instructions and allocate temps as needed directly, as you suggested above. @alexcrichton hit a similar issue with is stack-check PR (#1573). It's my design flaw originally (sorry!) so I can take this as a followup item once that PR and this one land.

[bnjbvr]

I looked into this, and I could make it work as discussed, but it puts the type parameter down to the implementation of store_stack_sp, which is unfortunate. Maybe we could revisit this later, and instead manually monomorphize the LowerCtx trait into its own impl Lower.

[cfallin]

Hmm, indeed; I want to try to keep things parameterized on the Lower trait (and not simplify to just the one impl) because it gives us flexibility later to e.g. be generic over multiple input IRs. This seems fine for now, I think.

[bnjbvr]

Thanks for the review! (Three more tests passing in wasmtime, yay!)

[cfallin]

The new version looks good, thanks!

I had been worried after commenting yesterday that it might not actually work to parameterize on LowerCtx in ABICall, because trait methods can't be generic; but that restriction only applies for dynamically-dispatched traits, namely the "object safety" rules, and we only statically monomorphize on ABICall (ABIBody on the other hand is held as dyn in a Box) so we just squeaked by, here.