Hi all,

This fixes an assertion that occurs when executing FRSQRTE, FRECPE, or FRECPX
on a signaling NaN when the CPU has default NaN mode enabled.

When attempting to silence the NaN, we hit an assertion that ensures that the
CPU FPU status does not have default_nan_mode set.

To avoid this, we check if default_nan_mode is set before trying to silence the
NaN.
What happens then is the instruction sets any flags because of the signaling
NaN, then gets the default NaN value due to default_nan_mode being set.

Thanks!
Joe

Joe Komlodi (1):
  target/arm: Check NaN mode before silencing NaN

 target/arm/helper-a64.c | 12 +++++++++---
 target/arm/vfp_helper.c | 24 ++++++++++++++++++------
 2 files changed, 27 insertions(+), 9 deletions(-)

-- 
2.7.4

On Sat, 26 Jun 2021 at 00:04, Joe Komlodi <joe.komlodi@xilinx.com> wrote:
>
> Hi all,
>
> This fixes an assertion that occurs when executing FRSQRTE, FRECPE, or FRECPX
> on a signaling NaN when the CPU has default NaN mode enabled.
>
> When attempting to silence the NaN, we hit an assertion that ensures that the
> CPU FPU status does not have default_nan_mode set.
>
> To avoid this, we check if default_nan_mode is set before trying to silence the
> NaN.
> What happens then is the instruction sets any flags because of the signaling
> NaN, then gets the default NaN value due to default_nan_mode being set.

Richard, Alex: what is the assertion trying to achieve ? It doesn't
seem entirely obvious to me that because we're in default-NaN mode
(which is a property of the *output* of FPU insns) that we should
blow up on calling float*_silence_nan() (which is typically an action
performed on the *input* of FPU insns).

This used to work fine, because we would have seen the assertions
when we tested the implementation of all these Arm insns...

If we do want to keep the assertion, somebody should audit the
other frontends that use float*_silence_nan() (i386, m68k, s390x)
to see if they also need updating.

thanks
-- PMM

On 6/28/21 7:54 AM, Peter Maydell wrote:
> Richard, Alex: what is the assertion trying to achieve ? It doesn't
> seem entirely obvious to me that because we're in default-NaN mode
> (which is a property of the *output* of FPU insns) that we should
> blow up on calling float*_silence_nan() (which is typically an action
> performed on the *input* of FPU insns).

This was in response to e9e5534ff30.

My assumption in adding the assert is that it was probably a configuration error.  If you 
disagree, I suppose we can revert it, as it's not critical.

> If we do want to keep the assertion, somebody should audit the
> other frontends that use float*_silence_nan() (i386, m68k, s390x)
> to see if they also need updating.

Easily done.  None of them ever set default_nan mode.


r~

On Mon, 28 Jun 2021 at 16:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/28/21 7:54 AM, Peter Maydell wrote:
> > Richard, Alex: what is the assertion trying to achieve ? It doesn't
> > seem entirely obvious to me that because we're in default-NaN mode
> > (which is a property of the *output* of FPU insns) that we should
> > blow up on calling float*_silence_nan() (which is typically an action
> > performed on the *input* of FPU insns).
>
> This was in response to e9e5534ff30.
>
> My assumption in adding the assert is that it was probably a configuration error.  If you
> disagree, I suppose we can revert it, as it's not critical.
>
> > If we do want to keep the assertion, somebody should audit the
> > other frontends that use float*_silence_nan() (i386, m68k, s390x)
> > to see if they also need updating.
>
> Easily done.  None of them ever set default_nan mode.

Hmm, I guess this was just Arm, then, and the current code
is silencing the NaN and then ignoring that result in favour
of the default NaN, which is a bit unnecessary. Plus, we have
this patch now thanks to Joe and we don't have the hypothetical
"drop the assert" patch :-)

Applied to target-arm.next, thanks.

-- PMM

On Mon, 28 Jun 2021 at 16:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/28/21 7:54 AM, Peter Maydell wrote:
> > Richard, Alex: what is the assertion trying to achieve ? It doesn't
> > seem entirely obvious to me that because we're in default-NaN mode
> > (which is a property of the *output* of FPU insns) that we should
> > blow up on calling float*_silence_nan() (which is typically an action
> > performed on the *input* of FPU insns).
>
> This was in response to e9e5534ff30.
>
> My assumption in adding the assert is that it was probably a configuration error.  If you
> disagree, I suppose we can revert it, as it's not critical.

I just ran across this again, in a different context. For MVE VMAXNMV
(which uses the "default FPSCR value"), I need to silence input SNaNs
before performing the max/min operation. The logical way to do that is
to call float*_silence_nan(). Except that that barfs on this assertion.

So I think that having run into this assertion() twice now it's
more awkward than helpful and I intend to put a patch deleting it
in the appropriate part of my next MVE series.

(In theory I could work around it by deliberately squashing the
"use the default NaN value flag" in a local copy of the fp_status,
but that seems like unnecessary work.)

thanks
-- PMM