Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert fma, valltrue & vanytrue to ISLE (AArch64) #4608

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

dheaton-arm
Copy link
Contributor

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

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
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. labels Aug 4, 2022
@@ -1,5 +1,7 @@
test interpret
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@afonso360 afonso360 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interpreter changes look good to me!

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This overall looks good. I have some comments/thoughts on the "in-place" / conditional-mod-operand aspect of VecRRR, but for merging this I think just some more comments will do, and we can clean up the (pre-existing) issue in a followup.

cranelift/codegen/src/isa/aarch64/inst.isle Outdated Show resolved Hide resolved
@@ -960,7 +960,7 @@ fn aarch64_get_operands<F: Fn(VReg) -> VReg>(inst: &Inst, collector: &mut Operan
&Inst::VecRRR {
alu_op, rd, rn, rm, ..
} => {
if alu_op == VecALUOp::Bsl {
if alu_op == VecALUOp::Bsl || alu_op == VecALUOp::Fmla {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

cranelift/codegen/src/isa/aarch64/inst.isle Show resolved Hide resolved
Copyright (c) 2022 Arm Limited
@cfallin cfallin merged commit eb332b8 into bytecodealliance:main Aug 5, 2022
@dheaton-arm dheaton-arm deleted the isle-fma branch August 8, 2022 08:35
dheaton-arm added a commit to dheaton-arm/wasmtime that referenced this pull request Aug 8, 2022
Separates the following opcodes for AArch64 into a separate `VecALUModOp` enum,
which is emitted via the `VecRRRMod` instruction. This separates vector ALU
instructions which modify a register from instructions which write to a new register:
- `Bsl`
- `Fmla`

Addresses [a discussion](bytecodealliance#4608 (comment)) in bytecodealliance#4608.

Copyright (c) 2022 Arm Limited
cfallin pushed a commit that referenced this pull request Aug 8, 2022
Separates the following opcodes for AArch64 into a separate `VecALUModOp` enum,
which is emitted via the `VecRRRMod` instruction. This separates vector ALU
instructions which modify a register from instructions which write to a new register:
- `Bsl`
- `Fmla`

Addresses [a discussion](#4608 (comment)) in #4608.

Copyright (c) 2022 Arm Limited
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants