Stream: git-wasmtime

Topic: wasmtime / PR #4608 Convert `fma`, `valltrue` & `vanytrue...

Wasmtime GitHub notifications bot (Aug 04 2022 at 13:23):

dheaton-arm opened PR #4608 from isle-fma to main:

Ported the existing implementations of the following opcodes to ISLE on
AArch64:

• fma

  • Introduced missing support for fma on vector values, as per the
    docs.

• valltrue

• vanytrue

Also fixed fcmp on scalar values in the interpreter, and enabled
interpreter tests in simd-fma.clif.

This introduces the FMLA machine instruction.

Copyright (c) 2022 Arm Limited

<!--

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.
-->

Wasmtime GitHub notifications bot (Aug 04 2022 at 13:27):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 13:27):

afonso360 created PR review comment:

fma is somewhat broken in the interpreter for the x86_64-pc-windows-gnu target (see #4517 for more details). We have a smaller subset of fma tests for the interpreter in a separate file.

Wasmtime GitHub notifications bot (Aug 04 2022 at 13:30):

afonso360 created PR review comment:

Although it looks like we don't have those problematic inputs in this test file, so the tests will probably pass.

Wasmtime GitHub notifications bot (Aug 04 2022 at 13:30):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 13:49):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 16:47):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 16:47):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 16:47):

cfallin created PR review comment:

Could we add a comment here (and also on bsl while we're at it) to note that the opcode must only be used with the vec_rrr_inplace constructor?

(Note below on a better approach here by separating the types for safety, but we don't need to do it right away)

Wasmtime GitHub notifications bot (Aug 04 2022 at 16:47):

cfallin created PR review comment:

Hmm, I'm not too happy about the conditional-mod here, but it's pre-existing, so I won't ask you to do anything in this PR. But noting it regardless: it would be better to split the case out to a new enum arm (VecRRRMod?) and then make the sub-opcode enum separate (VecALUOp / VecALUModOp maybe?) for more safety at the type level. (If you want to tackle that in a followup PR, I'd be happy to review!)

Wasmtime GitHub notifications bot (Aug 04 2022 at 16:47):

cfallin created PR review comment:

the src_dst here is actually a pure source, since we move out of it below, right? Could we call it src1 instead for clarity?

Wasmtime GitHub notifications bot (Aug 05 2022 at 09:06):

dheaton-arm updated PR #4608 from isle-fma to main.

Wasmtime GitHub notifications bot (Aug 05 2022 at 16:47):

cfallin merged PR #4608.

Last updated: Nov 22 2024 at 17:03 UTC