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 insimd-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.
-->
afonso360 submitted PR review.
afonso360 created PR review comment:
fma
is somewhat broken in the interpreter for thex86_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.
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.
afonso360 submitted PR review.
afonso360 submitted PR review.
cfallin submitted PR review.
cfallin submitted PR review.
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 thevec_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)
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!)
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 itsrc1
instead for clarity?
dheaton-arm updated PR #4608 from isle-fma
to main
.
cfallin merged PR #4608.
Last updated: Dec 23 2024 at 12:05 UTC