[PATCH 0/1] target/arm: Check NaN mode before silencing NaN

[PATCH 0/1] target/arm: Check NaN mode before silencing NaN

[Joe Komlodi]

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

[PATCH 1/1] target/arm: Check NaN mode before silencing NaN

[Joe Komlodi]

If the CPU is running in default NaN mode (FPCR.DN == 1) and we execute
FRSQRTE, FRECPE, or FRECPX with a signaling NaN, parts_silence_nan_frac() will
assert due to fpst->default_nan_mode being set.

To avoid this, we check to see what NaN mode we're running in before we call
floatxx_silence_nan().

Signed-off-by: Joe Komlodi <joe.komlodi@xilinx.com>
---
 target/arm/helper-a64.c | 12 +++++++++---
 target/arm/vfp_helper.c | 24 ++++++++++++++++++------
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 9cc3b06..ac5c445 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -365,7 +365,9 @@ uint32_t HELPER(frecpx_f16)(uint32_t a, void *fpstp)
         float16 nan = a;
         if (float16_is_signaling_nan(a, fpst)) {
             float_raise(float_flag_invalid, fpst);
-            nan = float16_silence_nan(a, fpst);
+            if (!fpst->default_nan_mode) {
+                nan = float16_silence_nan(a, fpst);
+            }
         }
         if (fpst->default_nan_mode) {
             nan = float16_default_nan(fpst);
@@ -396,7 +398,9 @@ float32 HELPER(frecpx_f32)(float32 a, void *fpstp)
         float32 nan = a;
         if (float32_is_signaling_nan(a, fpst)) {
             float_raise(float_flag_invalid, fpst);
-            nan = float32_silence_nan(a, fpst);
+            if (!fpst->default_nan_mode) {
+                nan = float32_silence_nan(a, fpst);
+            }
         }
         if (fpst->default_nan_mode) {
             nan = float32_default_nan(fpst);
@@ -427,7 +431,9 @@ float64 HELPER(frecpx_f64)(float64 a, void *fpstp)
         float64 nan = a;
         if (float64_is_signaling_nan(a, fpst)) {
             float_raise(float_flag_invalid, fpst);
-            nan = float64_silence_nan(a, fpst);
+            if (!fpst->default_nan_mode) {
+                nan = float64_silence_nan(a, fpst);
+            }
         }
         if (fpst->default_nan_mode) {
             nan = float64_default_nan(fpst);
diff --git a/target/arm/vfp_helper.c b/target/arm/vfp_helper.c
index 8a71660..24e3d82 100644
--- a/target/arm/vfp_helper.c
+++ b/target/arm/vfp_helper.c
@@ -671,7 +671,9 @@ uint32_t HELPER(recpe_f16)(uint32_t input, void *fpstp)
         float16 nan = f16;
         if (float16_is_signaling_nan(f16, fpst)) {
             float_raise(float_flag_invalid, fpst);
-            nan = float16_silence_nan(f16, fpst);
+            if (!fpst->default_nan_mode) {
+                nan = float16_silence_nan(f16, fpst);
+            }
         }
         if (fpst->default_nan_mode) {
             nan =  float16_default_nan(fpst);
@@ -719,7 +721,9 @@ float32 HELPER(recpe_f32)(float32 input, void *fpstp)
         float32 nan = f32;
         if (float32_is_signaling_nan(f32, fpst)) {
             float_raise(float_flag_invalid, fpst);
-            nan = float32_silence_nan(f32, fpst);
+            if (!fpst->default_nan_mode) {
+                nan = float32_silence_nan(f32, fpst);
+            }
         }
         if (fpst->default_nan_mode) {
             nan =  float32_default_nan(fpst);
@@ -767,7 +771,9 @@ float64 HELPER(recpe_f64)(float64 input, void *fpstp)
         float64 nan = f64;
         if (float64_is_signaling_nan(f64, fpst)) {
             float_raise(float_flag_invalid, fpst);
-            nan = float64_silence_nan(f64, fpst);
+            if (!fpst->default_nan_mode) {
+                nan = float64_silence_nan(f64, fpst);
+            }
         }
         if (fpst->default_nan_mode) {
             nan =  float64_default_nan(fpst);
@@ -866,7 +872,9 @@ uint32_t HELPER(rsqrte_f16)(uint32_t input, void *fpstp)
         float16 nan = f16;
         if (float16_is_signaling_nan(f16, s)) {
             float_raise(float_flag_invalid, s);
-            nan = float16_silence_nan(f16, s);
+            if (!s->default_nan_mode) {
+                nan = float16_silence_nan(f16, fpstp);
+            }
         }
         if (s->default_nan_mode) {
             nan =  float16_default_nan(s);
@@ -910,7 +918,9 @@ float32 HELPER(rsqrte_f32)(float32 input, void *fpstp)
         float32 nan = f32;
         if (float32_is_signaling_nan(f32, s)) {
             float_raise(float_flag_invalid, s);
-            nan = float32_silence_nan(f32, s);
+            if (!s->default_nan_mode) {
+                nan = float32_silence_nan(f32, fpstp);
+            }
         }
         if (s->default_nan_mode) {
             nan =  float32_default_nan(s);
@@ -953,7 +963,9 @@ float64 HELPER(rsqrte_f64)(float64 input, void *fpstp)
         float64 nan = f64;
         if (float64_is_signaling_nan(f64, s)) {
             float_raise(float_flag_invalid, s);
-            nan = float64_silence_nan(f64, s);
+            if (!s->default_nan_mode) {
+                nan = float64_silence_nan(f64, fpstp);
+            }
         }
         if (s->default_nan_mode) {
             nan =  float64_default_nan(s);
-- 
2.7.4

Re: [PATCH 1/1] target/arm: Check NaN mode before silencing NaN

[Richard Henderson]

On 6/25/21 4:02 PM, Joe Komlodi wrote:
> If the CPU is running in default NaN mode (FPCR.DN == 1) and we execute
> FRSQRTE, FRECPE, or FRECPX with a signaling NaN, parts_silence_nan_frac() will
> assert due to fpst->default_nan_mode being set.
> 
> To avoid this, we check to see what NaN mode we're running in before we call
> floatxx_silence_nan().
> 
> Signed-off-by: Joe Komlodi<joe.komlodi@xilinx.com>
> ---
>   target/arm/helper-a64.c | 12 +++++++++---
>   target/arm/vfp_helper.c | 24 ++++++++++++++++++------
>   2 files changed, 27 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/1] target/arm: Check NaN mode before silencing NaN

[Peter Maydell]

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

Re: [PATCH 0/1] target/arm: Check NaN mode before silencing NaN

[Richard Henderson]

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~

Re: [PATCH 0/1] target/arm: Check NaN mode before silencing NaN

[Peter Maydell]

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

Re: [PATCH 0/1] target/arm: Check NaN mode before silencing NaN

[Peter Maydell]

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