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.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin requested julian-seward1 for a review on PR #2128.
bjorn3 submitted PR Review.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
ty.lane_type().is_float()
bjorn3 created PR Review Comment:
usize::from(ty.bits())
bjorn3 created PR Review Comment:
Can you add support for
ArgumentPurpose::StructArgument(size)
?
bjorn3 created PR Review Comment:
!ty.is_vector && (ty.is_bool() || ty.is_int() || ty.is_ref())
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
See my top-level comments regarding
MemArg
.
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
nit: "The MOV" --> either "MOV" or "The MOV insn"
julian-seward1 submitted PR Review.
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 fromir::Type
toRegClass
. Thenin_int_reg(ty)
becomesclass_for_ty(ty) == RegClass::I64
.
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
Maybe name
is_callee_save_reg
would be clearer, when reading use points?
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
Can these be unstable sorts, in the interest of cranking every last nanosecond out of the machine?
julian-seward1 submitted PR Review.
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
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
We're sure that
usize::from(ty.bits())
actucally produces the same numbers, yes?
julian-seward1 submitted PR Review.
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?
julian-seward1 submitted PR Review.
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
andis_ref
covering all possible types exactly once.
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
Could this just be
StackArg
? If an arg is on the stack it is definitely in memory.
julian-seward1 submitted PR Review.
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).
julian-seward1 submitted PR Review.
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.
julian-seward1 submitted PR Review.
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?
julian-seward1 submitted PR Review.
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?
julian-seward1 submitted PR Review.
julian-seward1 edited PR Review Comment.
julian-seward1 submitted PR Review.
julian-seward1 submitted PR Review.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
For
IFLAGS
andFFLAGS
it always returns 0. Other than that everything produces the same value.
cfallin submitted PR Review.
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_*()
.
cfallin submitted PR Review.
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 :-)
cfallin submitted PR Review.
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...
cfallin submitted PR Review.
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.)
cfallin submitted PR Review.
cfallin created PR Review Comment:
Removed, actually, as we can use other existing helpers -- thanks!
cfallin submitted PR Review.
cfallin created PR Review Comment:
Actually calling it
StackAMode
as per the below (MemArg
->AMode
), thanks!
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.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
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.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin merged PR #2128.
julian-seward1 submitted PR Review.
julian-seward1 created PR Review Comment:
Oh, yes, I am all in favour of determinism. I retract my comment :-)
bjorn3 submitted PR Review.
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.
bjorn3 edited PR Review Comment.
Last updated: Dec 23 2024 at 12:05 UTC