Hi Richard,
On Wed, May 7, 2025 at 1:50 AM Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> On 5/6/25 08:26, WANG Rui wrote:
> > This patch fixes incorrect results for [xv]fnm{add,sub}.{s,d}
> > instructions when rounding toward zero, postive, negative.
> >
> > According to the LoongArch ISA specification, these instructions perform
> > a fused multiply-add followed by a negation of the final result.
> >
> > Previously, the sign inversion was applied before fused operation, which
> > interfered with rounding decisions that depend on the result sign --
> > leading to deviations from the expected behavior. This patch corrects
> > the implementation by applying the negation after fused multiply-add,
> > ensuring that rounding is performed on the correct intermediate result.
> >
> > Reported-by: mengqinggang <mengqingg...@loongson.cn>
> > Signed-off-by: WANG Rui <wang...@loongson.cn>
> > ---
> > target/loongarch/tcg/fpu_helper.c | 8 ++++++++
> > target/loongarch/tcg/vec_helper.c | 7 ++++++-
> > tests/tcg/loongarch64/Makefile.target | 2 ++
> > tests/tcg/loongarch64/test_fnmsub.c | 20 ++++++++++++++++++++
> > tests/tcg/loongarch64/test_vfnmsub.c | 27 +++++++++++++++++++++++++++
> > 5 files changed, 63 insertions(+), 1 deletion(-)
> > create mode 100644 tests/tcg/loongarch64/test_fnmsub.c
> > create mode 100644 tests/tcg/loongarch64/test_vfnmsub.c
> >
> > diff --git a/target/loongarch/tcg/fpu_helper.c
> > b/target/loongarch/tcg/fpu_helper.c
> > index fc3fd0561e..970b88ac56 100644
> > --- a/target/loongarch/tcg/fpu_helper.c
> > +++ b/target/loongarch/tcg/fpu_helper.c
> > @@ -389,9 +389,13 @@ uint64_t helper_fmuladd_s(CPULoongArchState *env,
> > uint64_t fj,
> > uint64_t fk, uint64_t fa, uint32_t flag)
> > {
> > uint64_t fd;
> > + uint32_t neg_res;
> >
> > + neg_res = flag & float_muladd_negate_result;
> > + flag ^= neg_res;
> > fd = nanbox_s(float32_muladd((uint32_t)fj, (uint32_t)fk,
> > (uint32_t)fa, flag, &env->fp_status));
> > + fd |= neg_res ? 0x80000000ULL : 0;
>
> |= cannot possibly be correct -- it would have to be ^=.
Good catch!
>
> Is this a problem with the sign of a NaN result? That's the only case that
> isn't handled
> by floatN_muladd. target/arm/ does not use the float_muladd_* flags for
> exactly this
> reason. The LoongArch manual has
>
> FNMSUB.D:
> FR[fd] = -FP64_fusedMultiplyAdd(FR[fj], FR[fk], -FR[fa])
In the case of `FNMSUB.D`, the result of instruction is computed as
the negation of `FP64_fusedMultiplyAdd(FR[fj], FR[fk], -FR[fa])`,
where `FP64_fusedMultiplyAdd()` performs a fused multiply-add
operation compliant with IEEE 754-2008. The negation is applied to the
fully rounded result of the fused operation, not to any intermediate
value. This behavior is specific to LoongArch and differs from other
architectures, which is why the existing `float_muladd_negate_result`
flag does not apply here.
Regards,
-hev
>
> which matches the language in the Arm manual. Which does suggest that the
> negation before
> on fa and the negation after on the result should be unconditional.
>
> However, for this test case, if I'm interpreting these numbers correctly,
>
> > + *(uint64_t *)&x = 0x4ff0000000000000UL;
> > + *(uint64_t *)&y = 0x4ff0000000000000UL;
>
> 0x1.0p256
>
> > + *(uint64_t *)&z = 0x2ff0000000000000UL;
>
> 0x1.0p-256
>
> > +
> > + fesetround(FE_DOWNWARD);
> > + asm("fnmsub.d %[x], %[x], %[y], %[z]\n\t"
> > + :[x]"+f"(x)
> > + :[y]"f"(y), [z]"f"(z));
> > +
> > + assert(*(uint64_t *)&x == 0xdfefffffffffffffUL);
>
> -(0x1.0p512 - epsilon)
>
> That really should have worked as-is. I'll have a quick look.
>
>
> r~
