Stream: git-wasmtime

Topic: wasmtime / PR #2959 aarch64 add basic i128 bit ops

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 11:04):

afonso360 opened PR #2959 from aarch64-i128-bit-ops to main:

Hey, some more ops implemented for i128 support.

I think the shifts can be reduced by a few instructions (especially the last csel), but I'm not seeing how right now.

I also didn't implement support for immlogic, changing it to support multiple registers didn't seem like a simple change.

This adds support for:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 11:08):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 11:08):

afonso360 created PR review comment:

@cfallin I pretty much copied the testing code from x64/shift-i128-run.clif. And added a few more test cases where my implementation had issues.

Do you think it would be a good idea to merge run tests so that they could be multi arch? I've been thinking about something along the lines of:

testfiles/runtests/i128-bitops.clif:

test run
target x86_64
target aarch64
target s390x

function %rest_of_the_tests() {}

This way we could reuse all of these test cases for all arches.

The down side of this is that we may have to be more granular with the testfiles. For example this file right here fails for x86_64 because some of the bit ops fail. So we would have to split this into shift_run and bitops_run or something along those lines.

What do you think?

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

afonso360 edited PR review comment.

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

afonso360 updated PR #2959 from aarch64-i128-bit-ops to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 11:16):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 11:16):

bjorn3 created PR review comment:

nit: missing trailing newline

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 11:36):

afonso360 updated PR #2959 from aarch64-i128-bit-ops to main.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

Yes, sharing run-tests makes a lot of sense! In general I actually want to try to shift tests from golden-code (compile tests) to golden-output (run tests) as this makes our suite more robust against cross-cutting backend changes, such as regalloc optimizations; so there are additional benefits, aside from sharing across architectures.

Could you be more specific about the failures you're seeing on x86_64, though? That's somewhat concerning -- unimplemented opcode, incorrect result, or something else?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 19:51):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 19:51):

afonso360 created PR review comment:

Sure, band_not is failling on this assert.

BorNot / BxorNot / Cls (not on this PR yet) are not implemented at all.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 19:51):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 19:51):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 20:34):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2021 at 20:34):

cfallin created PR review comment:

Ah, OK, for tests like those for BandNot where we don't have an implementation on x86_64, we can just omit the target line in the test file, perhaps with a comment saying "not yet implemented on $PLATFORM".

view this post on Zulip Wasmtime GitHub notifications bot (Jun 07 2021 at 13:19):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 07 2021 at 13:19):

akirilov-arm created PR review comment:

You are right that this instruction sequence can be improved - see what Clang is generating in the same situation here.

A similar improvement is possible for the right shifts.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 08 2021 at 11:36):

afonso360 updated PR #2959 from aarch64-i128-bit-ops to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 08 2021 at 11:38):

afonso360 created PR review comment:

Oops, i should have checked that.

I've fixed the code so that it now emits a similar sequence to what clang is doing.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 08 2021 at 11:38):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 08 2021 at 21:10):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2021 at 18:45):

cfallin submitted PR review.

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

afonso360 updated PR #2959 from aarch64-i128-bit-ops to main.

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

afonso360 updated PR #2959 from aarch64-i128-bit-ops to main.

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

afonso360 updated PR #2959 from aarch64-i128-bit-ops to main.

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

afonso360 updated PR #2959 from aarch64-i128-bit-ops to main.

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

cfallin merged PR #2959.

Last updated: Nov 22 2024 at 17:03 UTC