wasmtime / PR #2128 Refactor AArch64 ABI support to extra... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2128 Refactor AArch64 ABI support to extra...

Wasmtime GitHub notifications bot (Aug 13 2020 at 04:06):

cfallin opened PR #2128 from machinst-abi-refactor-2 to main:

We have observed that the ABI implementations for AArch64 and x64 are
very similar; in fact, x64's implementation started as a modified copy
of AArch64's implementation. This is an artifact of both a similar ABI
(both machines pass args and return values in registers first, then the
stack, and both machines give considerable freedom with stack-frame
layout) and a too-low-level ABI abstraction in the existing design. For
machines that fit the mainstream or most common ABI-design idioms, we
should be able to do much better.

This commit factors AArch64 into machine-specific and
machine-independent parts, but does not yet modify x64; that will come
next.

This should be completely neutral with respect to compile time and
generated code performance.

Wasmtime GitHub notifications bot (Aug 13 2020 at 04:06):

cfallin requested julian-seward1 for a review on PR #2128.

Wasmtime GitHub notifications bot (Aug 13 2020 at 08:46):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 13 2020 at 08:46):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 13 2020 at 08:46):

bjorn3 created PR Review Comment:

    ty.lane_type().is_float()

Wasmtime GitHub notifications bot (Aug 13 2020 at 08:46):

bjorn3 created PR Review Comment:

    usize::from(ty.bits())

Wasmtime GitHub notifications bot (Aug 13 2020 at 08:46):

bjorn3 created PR Review Comment:

Can you add support for ArgumentPurpose::StructArgument(size)?

Wasmtime GitHub notifications bot (Aug 13 2020 at 08:46):

bjorn3 created PR Review Comment:

    !ty.is_vector && (ty.is_bool() || ty.is_int() || ty.is_ref())

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:12):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:12):

julian-seward1 created PR Review Comment:

See my top-level comments regarding MemArg.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:15):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:15):

julian-seward1 created PR Review Comment:

nit: "The MOV" --> either "MOV" or "The MOV insn"

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:20):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:20):

julian-seward1 created PR Review Comment:

One way to generalise these kinds of target-specific in_meh_reg predicates is to change them into functions from ir::Type to RegClass. Then in_int_reg(ty) becomes class_for_ty(ty) == RegClass::I64.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:20):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:20):

julian-seward1 created PR Review Comment:

Maybe name is_callee_save_reg would be clearer, when reading use points?

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:21):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:21):

julian-seward1 created PR Review Comment:

Can these be unstable sorts, in the interest of cranking every last nanosecond out of the machine?

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:29):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:29):

julian-seward1 created PR Review Comment:

The general comment structure of "this is for AArch64 and X64" gives me some concern. These comments are soon going to be out of date and hence confusing, once ports to other targets exist. I would prefer that

the implementation is introduced as "a vanilla modern ABI, in which some the first few (possibly zero) args are passed in registers, and the rest on the stack". I think that will cover most bases in the short/medium term, at least until we have to deal with the POWER64 ABIs, which are pretty weird due to the TOC requirement.

references to "AArch64 and X64" are removed

in the comment(s), instantiations/descriptions for each architecture are laid out explicitly, so that when adding a new architecture, the relevant pieces of the comments can be updated

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:31):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:31):

julian-seward1 created PR Review Comment:

We're sure that usize::from(ty.bits()) actucally produces the same numbers, yes?

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:35):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:35):

julian-seward1 created PR Review Comment:

Could this function be renamed to something that exposes the reason for the query? I mean, ty_is_int is, well, why do we care? If it were (eg) param_of_this_ty_goes_in_intreg then that would be clearer. But that's a decision that's surely per-target? Why would target-independent code want to know about the apparent int-ness of a type?

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:38):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:38):

julian-seward1 created PR Review Comment:

Personally I prefer the original, since it involves less indirection for the reader and doesn't make any assumptions about is_vector, is_bool, is_int and is_ref covering all possible types exactly once.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:39):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:39):

julian-seward1 created PR Review Comment:

Could this just be StackArg? If an arg is on the stack it is definitely in memory.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:40):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:40):

julian-seward1 created PR Review Comment:

Could you clarify a bit what gen_clobber_save/restore do? I can kinda guess, but a bit more clarity here would be nice (maybe just an extra sentence).

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:44):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:44):

julian-seward1 created PR Review Comment:

This doesn't say anything about alignment spacers pushed before the outbound args are pushed, so as to give the correct alignment for SP before making a call. Does it need to? For SM's wasm baseline compiler this was certainly A Thing that we had to work through (for stackmap generation, at least), and it was most confusingful.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:45):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:45):

julian-seward1 created PR Review Comment:

nano-nit, but .. when I saw nominal-SP I thought "nominal minus SP, what?!" Can you remove the - so as to make it consistent with the definition point on the next line?

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:49):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:49):

julian-seward1 created PR Review Comment:

Re the * 8, here and everywhere else .. this makes me nervous regarding 64/32-bit cleanness when we inevitable come to having to support a 32 bit target. Would you consider finding all places in the diff where there is an assumption that the word size is 64-bit, and adding a warning comment line // WARNING 32-bit unclean or some such? For the benefit of future porters?

Wasmtime GitHub notifications bot (Aug 14 2020 at 06:56):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 07:00):

julian-seward1 edited PR Review Comment.

Wasmtime GitHub notifications bot (Aug 14 2020 at 07:01):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 17:28):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 17:35):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 17:35):

bjorn3 created PR Review Comment:

https://github.com/bytecodealliance/wasmtime/blob/a577667f9fcc29528532f11a17e5e747cba7b13e/cranelift/codegen/src/ir/types.rs#L246-L248

https://github.com/bytecodealliance/wasmtime/blob/a577667f9fcc29528532f11a17e5e747cba7b13e/cranelift/codegen/src/ir/types.rs#L69-L80

For IFLAGS and FFLAGS it always returns 0. Other than that everything produces the same value.

Wasmtime GitHub notifications bot (Aug 14 2020 at 20:38):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 20:38):

cfallin created PR Review Comment:

Hmm, I think I'm going to go with @bjorn3's suggestion here, just because we've had several instances of merge conflicts in these match-on-type statements when someone adds support for a new vector type. It seems to me that it's better to have a single source of truth for the type categories, which is what we have with Type::is_*().

Wasmtime GitHub notifications bot (Aug 14 2020 at 20:40):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 20:40):

cfallin created PR Review Comment:

This is a separate issue, IMHO, that can be done in another PR. There's a bunch of other stuff I need to get to before supporting struct args, but I'd be happy to review a PR for this :-)

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:00):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:00):

cfallin created PR Review Comment:

I thought about that, but IMHO it's better to have truly deterministic output -- not doing so could create all sorts of weird testing issues, problems with caching at various levels, etc. We expect these lists to be short (at most the number of machine registers) so the difference shouldn't be too large...

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:01):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:01):

cfallin created PR Review Comment:

(And to be clear, the nondeterminism is otherwise new with this patch, because it changes the iteration to be directly over the Set rather than over the set-converted-to-vec, and the iteration order is unspecified.)

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:01):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:01):

cfallin created PR Review Comment:

Removed, actually, as we can use other existing helpers -- thanks!

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:17):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:17):

cfallin created PR Review Comment:

Actually calling it StackAMode as per the below (MemArg -> AMode), thanks!

Wasmtime GitHub notifications bot (Aug 14 2020 at 21:37):

cfallin updated PR #2128 from machinst-abi-refactor-2 to main:

We have observed that the ABI implementations for AArch64 and x64 are
very similar; in fact, x64's implementation started as a modified copy
of AArch64's implementation. This is an artifact of both a similar ABI
(both machines pass args and return values in registers first, then the
stack, and both machines give considerable freedom with stack-frame
layout) and a too-low-level ABI abstraction in the existing design. For
machines that fit the mainstream or most common ABI-design idioms, we
should be able to do much better.

This commit factors AArch64 into machine-specific and
machine-independent parts, but does not yet modify x64; that will come
next.

This should be completely neutral with respect to compile time and
generated code performance.

Wasmtime GitHub notifications bot (Aug 14 2020 at 23:27):

cfallin updated PR #2128 from machinst-abi-refactor-2 to main:

We have observed that the ABI implementations for AArch64 and x64 are
very similar; in fact, x64's implementation started as a modified copy
of AArch64's implementation. This is an artifact of both a similar ABI
(both machines pass args and return values in registers first, then the
stack, and both machines give considerable freedom with stack-frame
layout) and a too-low-level ABI abstraction in the existing design. For
machines that fit the mainstream or most common ABI-design idioms, we
should be able to do much better.

This commit factors AArch64 into machine-specific and
machine-independent parts, but does not yet modify x64; that will come
next.

This should be completely neutral with respect to compile time and
generated code performance.

Wasmtime GitHub notifications bot (Aug 15 2020 at 00:08):

cfallin merged PR #2128.

Wasmtime GitHub notifications bot (Aug 15 2020 at 09:24):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 15 2020 at 09:24):

julian-seward1 created PR Review Comment:

Oh, yes, I am all in favour of determinism. I retract my comment :-)

Wasmtime GitHub notifications bot (Aug 15 2020 at 11:37):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 15 2020 at 11:37):

bjorn3 created PR Review Comment:

unstable_sort_by_key doesn't affect determinism. It only causes a difference when multiple values have the same sort key. In that case the order would be dependent on the internals of the sorting algorithm, but not any random source. sort_by_key on the other hand guarantees that multiple values with the same sort key are kept in the same order.

Wasmtime GitHub notifications bot (Aug 15 2020 at 11:40):

bjorn3 edited PR Review Comment.

Last updated: Nov 22 2024 at 17:03 UTC