Stream: git-wasmtime

Topic: wasmtime / PR #2892 Handle i128 arguments in the aarch64 ABI

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2021 at 09:37):

afonso360 opened PR #2892 from aarch64-multireg-args to main:

When dealing with params that need to be split, we follow the arch64 ABI and split the value in two, and make sure that start that argument in an even numbered xN register.

The apple ABI does not require this, so on those platforms, we start params anywhere.

This is a draft because I'm not really sure how to implement tests for this.

It would be really nice if we could implement tests like the riscv backend does, but the aarch64 doesn't seem to produce that output in legaliztion?

I've had a look at the riscv ABI implementation and it looks like they use TargetIsa. Only the riscv and the x86 backend use that. So I'm not sure if that is part of the old backends.

Otherwise I think the best way to test this might be to just do tests and call compute_arg_locs directly.
I can also go a bit further and implement the lowering for multiple registers and then do the tests in clif files if you would prefer that.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2021 at 11:52):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2021 at 11:54):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2021 at 12:10):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2021 at 12:16):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2021 at 12:51):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2021 at 13:41):

afonso360 has marked PR #2892 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 00:31):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 00:31):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 00:31):

akirilov-arm created PR review comment:

This document is already outdated - it doesn't cover SVE, for example. Instead, point to the authoritative source - you can find links both to a HTML and a PDF version for a particular release; alternatively, use the HTML version from the main branch.

The same link is also used on line 190 - you should change it too.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 00:31):

akirilov-arm created PR review comment:

I suppose that you changed the code structure from a series of ifs and elses to usage of continue to make it easier to read (personally, I'd have kept the difference smaller), but IMHO checking the same condition twice looks weird, in spite of the negation the second time, and this block is short enough to just use an else block.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 00:31):

akirilov-arm created PR review comment:

Couldn't we just calculate this on line 415?

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 00:31):

akirilov-arm created PR review comment:

This should be The Apple AArch64 ABI.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 00:34):

akirilov-arm edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2021 at 10:04):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 08:29):

afonso360 requested akirilov-arm for a review on PR #2892.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm created PR review comment:

This function should be called extend_reg().

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm created PR review comment:

On second thought, this should really be a link to a specific release, the latest one being 2021Q1. The section number is no longer correct - it is 6.4 now. Here's a direct link to the specific section in the HTML version.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm created PR review comment:

Ditto; here's another direct link.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm created PR review comment:

Future-proof this by making the type of c to be u128. Some of the operations in the body of the function will need adjustments too; same for the comment before the function.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm created PR review comment:

How about returning (in_regs, ty, is_const) and then removing lines 269 and 274?

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm created PR review comment:

Use the plural:

... value into newly-allocated temporary registers, returning these registers.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm created PR review comment:

This should say put_input_in_regs.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2021 at 19:47):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 11:09):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 12:11):

afonso360 requested akirilov-arm for a review on PR #2892.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 15:50):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 15:50):

akirilov-arm created PR review comment:

Ditto.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 15:50):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 15:50):

akirilov-arm created PR review comment:

Sorry, I forgot that this function has other callers than just put_input_in_reg() and the new signature is more inconvenient for them - this should just return ValueRegs<Reg>, so that line 1575 in cranelift/codegen/src/isa/aarch64/lower_inst.rs, for example, doesn't ignore anything, and then the function body should be moved to another internal, non-public function, which is also going to be called by put_input_in_reg() directly.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 15:50):

akirilov-arm created PR review comment:

My bad - this should use the tag, 2021Q1, not the commit ID, i.e. it should look like this.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 16:06):

afonso360 updated PR #2892 from aarch64-multireg-args to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 16:06):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 16:06):

afonso360 created PR review comment:

This feels a lot less awkward than what we had before

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 16:07):

afonso360 requested akirilov-arm for a review on PR #2892.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 16:22):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 16:22):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2021 at 23:57):

cfallin merged PR #2892.

Last updated: Dec 23 2024 at 12:05 UTC