[Qemu-devel] [PATCH 00/12] [PULL] ARM fixes

[Qemu-devel] [PATCH 00/12] [PULL] ARM fixes

[Peter Maydell]

Anthony,

This is a pull request for patches which fix problems with the
generated code for ARM targets. They've been sent to the list
previously, and I've reviewed and tested them. Could they
be merged, please?


The following changes since commit 4e02d460dd4b60847a1e8b689cb676e3e1f3de95:
  Stefan Hajnoczi (1):
        virtio-pci: Convert fprintf() to error_report()

are available in the git repository at:

  git://git.linaro.org/qemu/qemu-arm.git for-anthony

Adam Lackorzynski (1):
      target-arm: Handle 'smc' as an undefined instruction

Johan Bengtsson (2):
      target-arm: Add support for PKHxx in thumb2
      target-arm: Fix mixup in decoding of saturating add and sub

Peter Maydell (9):
      ARM: Fix decoding of VFP forms of VCVT between float and int/fixed
      ARM: Fix decoding of Neon forms of VCVT between float and fixed point
      ARM: Fix sense of to_integer bit in Neon VCVT float/int conversion
      ARM: Return correct result for float-to-integer conversion of NaN
      ARM: Return correct result for single<->double conversion of NaN
      ARM: Ignore top 16 bits when doing VCVT from 16 bit fixed point
      softfloat: Add float/double to 16 bit integer conversion functions
      ARM: Implement VCVT to 16 bit integer using new softfloat routines
      ARM: fix ldrexd/strexd

 fpu/softfloat.c        |  136 ++++++++++++++++++++++++++++++++++++++++++++++++
 fpu/softfloat.h        |    4 ++
 linux-user/main.c      |    2 +-
 target-arm/helper.c    |   66 ++++++++++++++++++++++--
 target-arm/translate.c |  119 +++++++++++++++++++++++++++++-------------
 5 files changed, 285 insertions(+), 42 deletions(-)

[Qemu-devel] [PATCH 01/12] target-arm: Add support for PKHxx in thumb2

[Peter Maydell]

From: Johan Bengtsson <teofrastius@gmail.com>

The PKHxx instructions were not recognized by the thumb2 decoder. The
solution provided in this changeset is identical to the arm-mode
implementation.

Signed-off-by: Johan Bengtsson <teofrastius@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   63 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 99464ab..183928b 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7601,27 +7601,54 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
             }
         }
         break;
-    case 5: /* Data processing register constant shift.  */
-        if (rn == 15) {
-            tmp = new_tmp();
-            tcg_gen_movi_i32(tmp, 0);
-        } else {
-            tmp = load_reg(s, rn);
-        }
-        tmp2 = load_reg(s, rm);
+    case 5:
+
         op = (insn >> 21) & 0xf;
-        shiftop = (insn >> 4) & 3;
-        shift = ((insn >> 6) & 3) | ((insn >> 10) & 0x1c);
-        conds = (insn & (1 << 20)) != 0;
-        logic_cc = (conds && thumb2_logic_op(op));
-        gen_arm_shift_im(tmp2, shiftop, shift, logic_cc);
-        if (gen_thumb2_data_op(s, op, conds, 0, tmp, tmp2))
-            goto illegal_op;
-        dead_tmp(tmp2);
-        if (rd != 15) {
+        if (op == 6) {
+            /* Halfword pack.  */
+            tmp = load_reg(s, rn);
+            tmp2 = load_reg(s, rm);
+            shift = ((insn >> 10) & 0x1c) | ((insn >> 6) & 0x3);
+            if (insn & (1 << 5)) {
+                /* pkhtb */
+                if (shift == 0)
+                    shift = 31;
+                tcg_gen_sari_i32(tmp2, tmp2, shift);
+                tcg_gen_andi_i32(tmp, tmp, 0xffff0000);
+                tcg_gen_ext16u_i32(tmp2, tmp2);
+            } else {
+                /* pkhbt */
+                if (shift)
+                    tcg_gen_shli_i32(tmp2, tmp2, shift);
+                tcg_gen_ext16u_i32(tmp, tmp);
+                tcg_gen_andi_i32(tmp2, tmp2, 0xffff0000);
+            }
+            tcg_gen_or_i32(tmp, tmp, tmp2);
+            dead_tmp(tmp2);
             store_reg(s, rd, tmp);
         } else {
-            dead_tmp(tmp);
+            /* Data processing register constant shift.  */
+            if (rn == 15) {
+                tmp = new_tmp();
+                tcg_gen_movi_i32(tmp, 0);
+            } else {
+                tmp = load_reg(s, rn);
+            }
+            tmp2 = load_reg(s, rm);
+
+            shiftop = (insn >> 4) & 3;
+            shift = ((insn >> 6) & 3) | ((insn >> 10) & 0x1c);
+            conds = (insn & (1 << 20)) != 0;
+            logic_cc = (conds && thumb2_logic_op(op));
+            gen_arm_shift_im(tmp2, shiftop, shift, logic_cc);
+            if (gen_thumb2_data_op(s, op, conds, 0, tmp, tmp2))
+                goto illegal_op;
+            dead_tmp(tmp2);
+            if (rd != 15) {
+                store_reg(s, rd, tmp);
+            } else {
+                dead_tmp(tmp);
+            }
         }
         break;
     case 13: /* Misc data processing.  */
-- 
1.6.3.3

[Qemu-devel] [PATCH 02/12] target-arm: Fix mixup in decoding of saturating add and sub

[Peter Maydell]

From: Johan Bengtsson <teofrastius@gmail.com>

The thumb2 decoder contained a mixup between the bit controlling
doubling and the bit controlling if the operation was an add or a sub.

Signed-off-by: Johan Bengtsson <teofrastius@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 183928b..947de6d 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7713,9 +7713,9 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
                 /* Saturating add/subtract.  */
                 tmp = load_reg(s, rn);
                 tmp2 = load_reg(s, rm);
-                if (op & 2)
-                    gen_helper_double_saturate(tmp, tmp);
                 if (op & 1)
+                    gen_helper_double_saturate(tmp, tmp);
+                if (op & 2)
                     gen_helper_sub_saturate(tmp, tmp2, tmp);
                 else
                     gen_helper_add_saturate(tmp, tmp, tmp2);
-- 
1.6.3.3

[Qemu-devel] [PATCH 03/12] target-arm: Handle 'smc' as an undefined instruction

[Peter Maydell]

From: Adam Lackorzynski <adam@os.inf.tu-dresden.de>

Refine check on bkpt so that smc and undefined instruction encodings are
handled as an undefined instruction and trap.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 947de6d..bf1e643 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6346,7 +6346,14 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
             dead_tmp(tmp2);
             store_reg(s, rd, tmp);
             break;
-        case 7: /* bkpt */
+        case 7:
+            /* SMC instruction (op1 == 3)
+               and undefined instructions (op1 == 0 || op1 == 2)
+               will trap */
+            if (op1 != 1) {
+                goto illegal_op;
+            }
+            /* bkpt */
             gen_set_condexec(s);
             gen_set_pc_im(s->pc - 4);
             gen_exception(EXCP_BKPT);
-- 
1.6.3.3

[Qemu-devel] [PATCH 04/12] ARM: Fix decoding of VFP forms of VCVT between float and int/fixed

[Peter Maydell]

Correct the decoding of source and destination registers
for the VFP forms of the VCVT instructions which convert
between floating point and integer or fixed-point.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index bf1e643..07a51ab 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -2870,16 +2870,18 @@ static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                     VFP_DREG_N(rn, insn);
                 }
 
-                if (op == 15 && (rn == 15 || rn > 17)) {
+                if (op == 15 && (rn == 15 || ((rn & 0x1c) == 0x18))) {
                     /* Integer or single precision destination.  */
                     rd = VFP_SREG_D(insn);
                 } else {
                     VFP_DREG_D(rd, insn);
                 }
-
-                if (op == 15 && (rn == 16 || rn == 17)) {
-                    /* Integer source.  */
-                    rm = ((insn << 1) & 0x1e) | ((insn >> 5) & 1);
+                if (op == 15 &&
+                    (((rn & 0x1c) == 0x10) || ((rn & 0x14) == 0x14))) {
+                    /* VCVT from int is always from S reg regardless of dp bit.
+                     * VCVT with immediate frac_bits has same format as SREG_M
+                     */
+                    rm = VFP_SREG_M(insn);
                 } else {
                     VFP_DREG_M(rm, insn);
                 }
@@ -2891,6 +2893,9 @@ static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 } else {
                     rd = VFP_SREG_D(insn);
                 }
+                /* NB that we implicitly rely on the encoding for the frac_bits
+                 * in VCVT of fixed to float being the same as that of SREG_M.
+                 */
                 rm = VFP_SREG_M(insn);
             }
 
@@ -3179,8 +3184,8 @@ static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 /* Write back the result.  */
                 if (op == 15 && (rn >= 8 && rn <= 11))
                     ; /* Comparison, do nothing.  */
-                else if (op == 15 && rn > 17)
-                    /* Integer result.  */
+                else if (op == 15 && dp && ((rn & 0x1c) == 0x18))
+                    /* VCVT double to int: always integer result. */
                     gen_mov_vreg_F0(0, rd);
                 else if (op == 15 && rn == 15)
                     /* conversion */
-- 
1.6.3.3

[Qemu-devel] [PATCH 05/12] ARM: Fix decoding of Neon forms of VCVT between float and fixed point

[Peter Maydell]

Fix errors in the decoding of the Neon forms of fixed-point VCVT:
 * fixed-point VCVT is op 14 and 15, not 15 and 16
 * the fbits immediate field was being misinterpreted
 * the sense of the to_fixed bit was inverted

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 07a51ab..6c19f97 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4850,11 +4850,15 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                     }
                     neon_store_reg64(cpu_V0, rd + pass);
                 }
-            } else if (op == 15 || op == 16) {
+            } else if (op >= 14) {
                 /* VCVT fixed-point.  */
+                /* We have already masked out the must-be-1 top bit of imm6,
+                 * hence this 32-shift where the ARM ARM has 64-imm6.
+                 */
+                shift = 32 - shift;
                 for (pass = 0; pass < (q ? 4 : 2); pass++) {
                     tcg_gen_ld_f32(cpu_F0s, cpu_env, neon_reg_offset(rm, pass));
-                    if (op & 1) {
+                    if (!(op & 1)) {
                         if (u)
                             gen_vfp_ulto(0, shift);
                         else
-- 
1.6.3.3

[Qemu-devel] [PATCH 06/12] ARM: Fix sense of to_integer bit in Neon VCVT float/int conversion

[Peter Maydell]

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 6c19f97..f018653 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -5664,16 +5664,16 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                             gen_helper_rsqrte_f32(cpu_F0s, cpu_F0s, cpu_env);
                             break;
                         case 60: /* VCVT.F32.S32 */
-                            gen_vfp_tosiz(0);
+                            gen_vfp_sito(0);
                             break;
                         case 61: /* VCVT.F32.U32 */
-                            gen_vfp_touiz(0);
+                            gen_vfp_uito(0);
                             break;
                         case 62: /* VCVT.S32.F32 */
-                            gen_vfp_sito(0);
+                            gen_vfp_tosiz(0);
                             break;
                         case 63: /* VCVT.U32.F32 */
-                            gen_vfp_uito(0);
+                            gen_vfp_touiz(0);
                             break;
                         default:
                             /* Reserved: 21, 29, 39-56 */
-- 
1.6.3.3

[Qemu-devel] [PATCH 07/12] ARM: Return correct result for float-to-integer conversion of NaN

[Peter Maydell]

The ARM architecture mandates that converting a NaN value to
integer gives zero (if Invalid Operation FP exceptions are
not being trapped). This isn't the behaviour of the SoftFloat
library, so NaNs must be special-cased.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 996d40d..72ba314 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2445,43 +2445,84 @@ float64 VFP_HELPER(sito, d)(float32 x, CPUState *env)
 }
 
 /* Float to integer conversion.  */
+
+/* Helper routines to identify NaNs. Note that softfloat's
+ * floatxx_is_nan() actually only returns true for quiet NaNs.
+ * A NaN has an exponent field all 1s and a fraction field
+ * anything except all zeros. Conveniently we can detect this
+ * by masking out the sign bit and doing an unsigned comparison.
+ */
+static int float32_is_any_nan(float32 x)
+{
+    return ((float32_val(x) & ~(1 << 31)) > 0x7f800000UL);
+}
+
+static int float64_is_any_nan(float64 x)
+{
+    return ((float64_val(x) & ~(1ULL << 63)) > 0x7ff0000000000000ULL);
+}
+
 float32 VFP_HELPER(toui, s)(float32 x, CPUState *env)
 {
+    if (float32_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float32_to_uint32(x, &env->vfp.fp_status));
 }
 
 float32 VFP_HELPER(toui, d)(float64 x, CPUState *env)
 {
+    if (float64_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float64_to_uint32(x, &env->vfp.fp_status));
 }
 
 float32 VFP_HELPER(tosi, s)(float32 x, CPUState *env)
 {
+    if (float32_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float32_to_int32(x, &env->vfp.fp_status));
 }
 
 float32 VFP_HELPER(tosi, d)(float64 x, CPUState *env)
 {
+    if (float64_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float64_to_int32(x, &env->vfp.fp_status));
 }
 
 float32 VFP_HELPER(touiz, s)(float32 x, CPUState *env)
 {
+    if (float32_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float32_to_uint32_round_to_zero(x, &env->vfp.fp_status));
 }
 
 float32 VFP_HELPER(touiz, d)(float64 x, CPUState *env)
 {
+    if (float64_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float64_to_uint32_round_to_zero(x, &env->vfp.fp_status));
 }
 
 float32 VFP_HELPER(tosiz, s)(float32 x, CPUState *env)
 {
+    if (float32_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float32_to_int32_round_to_zero(x, &env->vfp.fp_status));
 }
 
 float32 VFP_HELPER(tosiz, d)(float64 x, CPUState *env)
 {
+    if (float64_is_any_nan(x)) {
+        return float32_zero;
+    }
     return vfp_itos(float64_to_int32_round_to_zero(x, &env->vfp.fp_status));
 }
 
@@ -2508,6 +2549,9 @@ ftype VFP_HELPER(name##to, p)(ftype x, uint32_t shift, CPUState *env) \
 ftype VFP_HELPER(to##name, p)(ftype x, uint32_t shift, CPUState *env) \
 { \
     ftype tmp; \
+    if (ftype##_is_any_nan(x)) { \
+        return ftype##_zero; \
+    } \
     tmp = ftype##_scalbn(x, shift, &env->vfp.fp_status); \
     return vfp_ito##p((itype)ftype##_to_##sign##int32_round_to_zero(tmp, \
         &env->vfp.fp_status)); \
-- 
1.6.3.3

[Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Peter Maydell]

The ARM ARM defines that if the input to a single<->double conversion
is a NaN then the output is always forced to be a quiet NaN by setting
the most significant bit of the fraction part.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 72ba314..356715c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2529,12 +2529,26 @@ float32 VFP_HELPER(tosiz, d)(float64 x, CPUState *env)
 /* floating point conversion */
 float64 VFP_HELPER(fcvtd, s)(float32 x, CPUState *env)
 {
-    return float32_to_float64(x, &env->vfp.fp_status);
+    float64 r = float32_to_float64(x, &env->vfp.fp_status);
+    /* ARM requires that S<->D conversion of any kind of NaN generates
+     * a quiet NaN by forcing the most significant frac bit to 1.
+     */
+    if (float64_is_signaling_nan(r)) {
+        return make_float64(float64_val(r) | (1LL << 51));
+    }
+    return r;
 }
 
 float32 VFP_HELPER(fcvts, d)(float64 x, CPUState *env)
 {
-    return float64_to_float32(x, &env->vfp.fp_status);
+    float32 r =  float64_to_float32(x, &env->vfp.fp_status);
+    /* ARM requires that S<->D conversion of any kind of NaN generates
+     * a quiet NaN by forcing the most significant frac bit to 1.
+     */
+    if (float32_is_signaling_nan(r)) {
+        return make_float32(float32_val(r) | (1 << 22));
+    }
+    return r;
 }
 
 /* VFP3 fixed point conversion.  */
-- 
1.6.3.3

[Qemu-devel] [PATCH 09/12] ARM: Ignore top 16 bits when doing VCVT from 16 bit fixed point

[Peter Maydell]

VCVT of 16 bit fixed point to float should ignore the top 16 bits
of the source register. Cast to int16_t and friends rather than
int16 -- the former is guaranteed exactly 16 bits wide where the
latter is merely at least 16 bits wide (and so is usually 32 bits).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 356715c..6a891da 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2556,7 +2556,7 @@ float32 VFP_HELPER(fcvts, d)(float64 x, CPUState *env)
 ftype VFP_HELPER(name##to, p)(ftype x, uint32_t shift, CPUState *env) \
 { \
     ftype tmp; \
-    tmp = sign##int32_to_##ftype ((itype)vfp_##p##toi(x), \
+    tmp = sign##int32_to_##ftype ((itype##_t)vfp_##p##toi(x), \
                                   &env->vfp.fp_status); \
     return ftype##_scalbn(tmp, -(int)shift, &env->vfp.fp_status); \
 } \
-- 
1.6.3.3

[Qemu-devel] [PATCH 10/12] softfloat: Add float/double to 16 bit integer conversion functions

[Peter Maydell]

The ARM architecture needs float/double to 16 bit integer conversions.
(The 32 bit versions aren't sufficient because of the requirement
to saturate at 16 bit MAXINT/MININT and to get the exception bits right.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 fpu/softfloat.c |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fpu/softfloat.h |    4 ++
 2 files changed, 140 insertions(+), 0 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 0b82797..6f5b05d 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -1355,6 +1355,55 @@ int32 float32_to_int32_round_to_zero( float32 a STATUS_PARAM )
 
 /*----------------------------------------------------------------------------
 | Returns the result of converting the single-precision floating-point value
+| `a' to the 16-bit two's complement integer format.  The conversion is
+| performed according to the IEC/IEEE Standard for Binary Floating-Point
+| Arithmetic, except that the conversion is always rounded toward zero.
+| If `a' is a NaN, the largest positive integer is returned.  Otherwise, if
+| the conversion overflows, the largest integer with the same sign as `a' is
+| returned.
+*----------------------------------------------------------------------------*/
+
+int16 float32_to_int16_round_to_zero( float32 a STATUS_PARAM )
+{
+    flag aSign;
+    int16 aExp, shiftCount;
+    bits32 aSig;
+    int32 z;
+
+    aSig = extractFloat32Frac( a );
+    aExp = extractFloat32Exp( a );
+    aSign = extractFloat32Sign( a );
+    shiftCount = aExp - 0x8E;
+    if ( 0 <= shiftCount ) {
+        if ( float32_val(a) != 0xC7000000 ) {
+            float_raise( float_flag_invalid STATUS_VAR);
+            if ( ! aSign || ( ( aExp == 0xFF ) && aSig ) ) {
+                return 0x7FFF;
+            }
+        }
+        return (sbits32) 0xffff8000;
+    }
+    else if ( aExp <= 0x7E ) {
+        if ( aExp | aSig ) {
+            STATUS(float_exception_flags) |= float_flag_inexact;
+        }
+        return 0;
+    }
+    shiftCount -= 0x10;
+    aSig = ( aSig | 0x00800000 )<<8;
+    z = aSig>>( - shiftCount );
+    if ( (bits32) ( aSig<<( shiftCount & 31 ) ) ) {
+        STATUS(float_exception_flags) |= float_flag_inexact;
+    }
+    if ( aSign ) {
+        z = - z;
+    }
+    return z;
+
+}
+
+/*----------------------------------------------------------------------------
+| Returns the result of converting the single-precision floating-point value
 | `a' to the 64-bit two's complement integer format.  The conversion is
 | performed according to the IEC/IEEE Standard for Binary Floating-Point
 | Arithmetic---which means in particular that the conversion is rounded
@@ -2412,6 +2461,57 @@ int32 float64_to_int32_round_to_zero( float64 a STATUS_PARAM )
 
 /*----------------------------------------------------------------------------
 | Returns the result of converting the double-precision floating-point value
+| `a' to the 16-bit two's complement integer format.  The conversion is
+| performed according to the IEC/IEEE Standard for Binary Floating-Point
+| Arithmetic, except that the conversion is always rounded toward zero.
+| If `a' is a NaN, the largest positive integer is returned.  Otherwise, if
+| the conversion overflows, the largest integer with the same sign as `a' is
+| returned.
+*----------------------------------------------------------------------------*/
+
+int16 float64_to_int16_round_to_zero( float64 a STATUS_PARAM )
+{
+    flag aSign;
+    int16 aExp, shiftCount;
+    bits64 aSig, savedASig;
+    int32 z;
+
+    aSig = extractFloat64Frac( a );
+    aExp = extractFloat64Exp( a );
+    aSign = extractFloat64Sign( a );
+    if ( 0x40E < aExp ) {
+        if ( ( aExp == 0x7FF ) && aSig ) {
+            aSign = 0;
+        }
+        goto invalid;
+    }
+    else if ( aExp < 0x3FF ) {
+        if ( aExp || aSig ) {
+            STATUS(float_exception_flags) |= float_flag_inexact;
+        }
+        return 0;
+    }
+    aSig |= LIT64( 0x0010000000000000 );
+    shiftCount = 0x433 - aExp;
+    savedASig = aSig;
+    aSig >>= shiftCount;
+    z = aSig;
+    if ( aSign ) {
+        z = - z;
+    }
+    if ( ( (int16_t)z < 0 ) ^ aSign ) {
+ invalid:
+        float_raise( float_flag_invalid STATUS_VAR);
+        return aSign ? (sbits32) 0xffff8000 : 0x7FFF;
+    }
+    if ( ( aSig<<shiftCount ) != savedASig ) {
+        STATUS(float_exception_flags) |= float_flag_inexact;
+    }
+    return z;
+}
+
+/*----------------------------------------------------------------------------
+| Returns the result of converting the double-precision floating-point value
 | `a' to the 64-bit two's complement integer format.  The conversion is
 | performed according to the IEC/IEEE Standard for Binary Floating-Point
 | Arithmetic---which means in particular that the conversion is rounded
@@ -5632,6 +5732,24 @@ unsigned int float32_to_uint32_round_to_zero( float32 a STATUS_PARAM )
     return res;
 }
 
+unsigned int float32_to_uint16_round_to_zero( float32 a STATUS_PARAM )
+{
+    int64_t v;
+    unsigned int res;
+
+    v = float32_to_int64_round_to_zero(a STATUS_VAR);
+    if (v < 0) {
+        res = 0;
+        float_raise( float_flag_invalid STATUS_VAR);
+    } else if (v > 0xffff) {
+        res = 0xffff;
+        float_raise( float_flag_invalid STATUS_VAR);
+    } else {
+        res = v;
+    }
+    return res;
+}
+
 unsigned int float64_to_uint32( float64 a STATUS_PARAM )
 {
     int64_t v;
@@ -5668,6 +5786,24 @@ unsigned int float64_to_uint32_round_to_zero( float64 a STATUS_PARAM )
     return res;
 }
 
+unsigned int float64_to_uint16_round_to_zero( float64 a STATUS_PARAM )
+{
+    int64_t v;
+    unsigned int res;
+
+    v = float64_to_int64_round_to_zero(a STATUS_VAR);
+    if (v < 0) {
+        res = 0;
+        float_raise( float_flag_invalid STATUS_VAR);
+    } else if (v > 0xffff) {
+        res = 0xffff;
+        float_raise( float_flag_invalid STATUS_VAR);
+    } else {
+        res = v;
+    }
+    return res;
+}
+
 /* FIXME: This looks broken.  */
 uint64_t float64_to_uint64 (float64 a STATUS_PARAM)
 {
diff --git a/fpu/softfloat.h b/fpu/softfloat.h
index 9528825..5d68a07 100644
--- a/fpu/softfloat.h
+++ b/fpu/softfloat.h
@@ -251,6 +251,8 @@ float32 float16_to_float32( bits16, flag STATUS_PARAM );
 /*----------------------------------------------------------------------------
 | Software IEC/IEEE single-precision conversion routines.
 *----------------------------------------------------------------------------*/
+int float32_to_int16_round_to_zero( float32 STATUS_PARAM );
+unsigned int float32_to_uint16_round_to_zero( float32 STATUS_PARAM );
 int float32_to_int32( float32 STATUS_PARAM );
 int float32_to_int32_round_to_zero( float32 STATUS_PARAM );
 unsigned int float32_to_uint32( float32 STATUS_PARAM );
@@ -321,6 +323,8 @@ INLINE int float32_is_zero(float32 a)
 /*----------------------------------------------------------------------------
 | Software IEC/IEEE double-precision conversion routines.
 *----------------------------------------------------------------------------*/
+int float64_to_int16_round_to_zero( float64 STATUS_PARAM );
+unsigned int float64_to_uint16_round_to_zero( float64 STATUS_PARAM );
 int float64_to_int32( float64 STATUS_PARAM );
 int float64_to_int32_round_to_zero( float64 STATUS_PARAM );
 unsigned int float64_to_uint32( float64 STATUS_PARAM );
-- 
1.6.3.3

[Qemu-devel] [PATCH 11/12] ARM: Implement VCVT to 16 bit integer using new softfloat routines

[Peter Maydell]

Use the softfloat conversion routines for conversion to 16 bit
integers, because just casting to a 16 bit type truncates the
value rather than saturating it at 16-bit MAXINT/MININT.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 6a891da..96d6fb4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2567,7 +2567,7 @@ ftype VFP_HELPER(to##name, p)(ftype x, uint32_t shift, CPUState *env) \
         return ftype##_zero; \
     } \
     tmp = ftype##_scalbn(x, shift, &env->vfp.fp_status); \
-    return vfp_ito##p((itype)ftype##_to_##sign##int32_round_to_zero(tmp, \
+    return vfp_ito##p(ftype##_to_##itype##_round_to_zero(tmp, \
         &env->vfp.fp_status)); \
 }
 
-- 
1.6.3.3

[Qemu-devel] [PATCH 12/12] ARM: fix ldrexd/strexd

[Peter Maydell]

Correct ldrexd and strexd code to always read and write the
high word of the 64-bit value from addr+4.
Also make ldrexd and strexd agree that for a 64 bit value the
address in env->exclusive_addr is that of the low word.

This fixes the issues reported in
https://bugs.launchpad.net/qemu/+bug/670883

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/main.c      |    2 +-
 target-arm/translate.c |    8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index dbba8be..274019f 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -589,7 +589,7 @@ static int do_strex(CPUARMState *env)
     }
     if (size == 3) {
         val = env->regs[(env->exclusive_info >> 12) & 0xf];
-        segv = put_user_u32(val, addr);
+        segv = put_user_u32(val, addr + 4);
         if (segv) {
             env->cp15.c6_data = addr + 4;
             goto done;
diff --git a/target-arm/translate.c b/target-arm/translate.c
index f018653..fc1d399 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -5935,8 +5935,10 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
     tcg_gen_mov_i32(cpu_exclusive_val, tmp);
     store_reg(s, rt, tmp);
     if (size == 3) {
-        tcg_gen_addi_i32(addr, addr, 4);
-        tmp = gen_ld32(addr, IS_USER(s));
+        TCGv tmp2 = new_tmp();
+        tcg_gen_addi_i32(tmp2, addr, 4);
+        tmp = gen_ld32(tmp2, IS_USER(s));
+        dead_tmp(tmp2);
         tcg_gen_mov_i32(cpu_exclusive_high, tmp);
         store_reg(s, rt2, tmp);
     }
@@ -5996,7 +5998,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
     if (size == 3) {
         TCGv tmp2 = new_tmp();
         tcg_gen_addi_i32(tmp2, addr, 4);
-        tmp = gen_ld32(addr, IS_USER(s));
+        tmp = gen_ld32(tmp2, IS_USER(s));
         dead_tmp(tmp2);
         tcg_gen_brcond_i32(TCG_COND_NE, tmp, cpu_exclusive_high, fail_label);
         dead_tmp(tmp);
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH 06/12] ARM: Fix sense of to_integer bit in Neon VCVT float/int conversion

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:45PM +0000, Peter Maydell wrote:
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 07/12] ARM: Return correct result for float-to-integer conversion of NaN

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:46PM +0000, Peter Maydell wrote:
> The ARM architecture mandates that converting a NaN value to
> integer gives zero (if Invalid Operation FP exceptions are
> not being trapped). This isn't the behaviour of the SoftFloat
> library, so NaNs must be special-cased.
> 
> +/* Helper routines to identify NaNs. Note that softfloat's
> + * floatxx_is_nan() actually only returns true for quiet NaNs.
> + * A NaN has an exponent field all 1s and a fraction field
> + * anything except all zeros. Conveniently we can detect this
> + * by masking out the sign bit and doing an unsigned comparison.
> + */
> +static int float32_is_any_nan(float32 x)
> +{
> +    return ((float32_val(x) & ~(1 << 31)) > 0x7f800000UL);
> +}
> +
> +static int float64_is_any_nan(float64 x)
> +{
> +    return ((float64_val(x) & ~(1ULL << 63)) > 0x7ff0000000000000ULL);
> +}

Why not just use:

static int float32_is_any_nan(float32 x)
{
  return float32_is_nan(x) || float32_is_signaling_nan(x);
}

and likewise for the 64-bit case?

-Nathan

Re: [Qemu-devel] [PATCH 07/12] ARM: Return correct result for float-to-integer conversion of NaN

[Peter Maydell]

On 29 November 2010 16:38, Nathan Froyd <froydnj@codesourcery.com> wrote:
>> +static int float32_is_any_nan(float32 x)
>> +{
>> +    return ((float32_val(x) & ~(1 << 31)) > 0x7f800000UL);
>> +}
>> +
>> +static int float64_is_any_nan(float64 x)
>> +{
>> +    return ((float64_val(x) & ~(1ULL << 63)) > 0x7ff0000000000000ULL);
>> +}
>
> Why not just use:
>
> static int float32_is_any_nan(float32 x)
> {
>  return float32_is_nan(x) || float32_is_signaling_nan(x);
> }
>
> and likewise for the 64-bit case?

That was what my first-pass patches did, but I
rewrote them this way because it seemed more
straightforward to just test for "is this a NaN" rather
than calling two other functions which each test for
"is this some subset of NaN space".

I suppose you could argue that softfloat ought to
have _is_nan() [with the semantics you'd expect
from the function name, not the ones it currently has!],
_is_signalling_nan() and _is_quiet_nan() functions
built in, but it doesn't...

-- PMM

Re: [Qemu-devel] [PATCH 07/12] ARM: Return correct result for float-to-integer conversion of NaN

[Nathan Froyd]

On Mon, Nov 29, 2010 at 04:49:24PM +0000, Peter Maydell wrote:
> On 29 November 2010 16:38, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > Why not just use:
> >
> > static int float32_is_any_nan(float32 x)
> > {
> >  return float32_is_nan(x) || float32_is_signaling_nan(x);
> > }
> >
> > and likewise for the 64-bit case?
> 
> That was what my first-pass patches did, but I
> rewrote them this way because it seemed more
> straightforward to just test for "is this a NaN" rather
> than calling two other functions which each test for
> "is this some subset of NaN space".
> 
> I suppose you could argue that softfloat ought to
> have _is_nan() [with the semantics you'd expect
> from the function name, not the ones it currently has!],
> _is_signalling_nan() and _is_quiet_nan() functions
> built in, but it doesn't...

I agree that the functions are poorly named.  I think it'd be better to
leave the bit-twiddling to the softfloat bits, though.  There's
precedent for the more verbose approach in other backends, too.

-Nathan

Re: [Qemu-devel] [PATCH 02/12] target-arm: Fix mixup in decoding of saturating add and sub

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:41PM +0000, Peter Maydell wrote:
> From: Johan Bengtsson <teofrastius@gmail.com>
> 
> The thumb2 decoder contained a mixup between the bit controlling
> doubling and the bit controlling if the operation was an add or a sub.
> 
> Signed-off-by: Johan Bengtsson <teofrastius@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 09/12] ARM: Ignore top 16 bits when doing VCVT from 16 bit fixed point

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:48PM +0000, Peter Maydell wrote:
> VCVT of 16 bit fixed point to float should ignore the top 16 bits
> of the source register. Cast to int16_t and friends rather than
> int16 -- the former is guaranteed exactly 16 bits wide where the
> latter is merely at least 16 bits wide (and so is usually 32 bits).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:47PM +0000, Peter Maydell wrote:
> The ARM ARM defines that if the input to a single<->double conversion
> is a NaN then the output is always forced to be a quiet NaN by setting
> the most significant bit of the fraction part.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> @@ -2529,12 +2529,26 @@ float32 VFP_HELPER(tosiz, d)(float64 x, CPUState *env)
>  /* floating point conversion */
>  float64 VFP_HELPER(fcvtd, s)(float32 x, CPUState *env)
>  {
> -    return float32_to_float64(x, &env->vfp.fp_status);
> +    float64 r = float32_to_float64(x, &env->vfp.fp_status);
> +    /* ARM requires that S<->D conversion of any kind of NaN generates
> +     * a quiet NaN by forcing the most significant frac bit to 1.
> +     */
> +    if (float64_is_signaling_nan(r)) {
> +        return make_float64(float64_val(r) | (1LL << 51));
> +    }
> +    return r;
>  }

As with other NaN-handling patches, I don't think the bit-twiddling here
is a good idea.  Having a float*_maybe_silence_nan function in softfloat
would be a better approach.

-Nathan

Re: [Qemu-devel] [PATCH 05/12] ARM: Fix decoding of Neon forms of VCVT between float and fixed point

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:44PM +0000, Peter Maydell wrote:
> Fix errors in the decoding of the Neon forms of fixed-point VCVT:
>  * fixed-point VCVT is op 14 and 15, not 15 and 16
>  * the fbits immediate field was being misinterpreted
>  * the sense of the to_fixed bit was inverted
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 03/12] target-arm: Handle 'smc' as an undefined instruction

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:42PM +0000, Peter Maydell wrote:
> From: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> 
> Refine check on bkpt so that smc and undefined instruction encodings are
> handled as an undefined instruction and trap.
> 
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 01/12] target-arm: Add support for PKHxx in thumb2

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:40PM +0000, Peter Maydell wrote:
> From: Johan Bengtsson <teofrastius@gmail.com>
> 
> The PKHxx instructions were not recognized by the thumb2 decoder. The
> solution provided in this changeset is identical to the arm-mode
> implementation.
> 
> Signed-off-by: Johan Bengtsson <teofrastius@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 10/12] softfloat: Add float/double to 16 bit integer conversion functions

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:49PM +0000, Peter Maydell wrote:
> The ARM architecture needs float/double to 16 bit integer conversions.
> (The 32 bit versions aren't sufficient because of the requirement
> to saturate at 16 bit MAXINT/MININT and to get the exception bits right.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 11/12] ARM: Implement VCVT to 16 bit integer using new softfloat routines

[Nathan Froyd]

On Tue, Nov 23, 2010 at 06:53:50PM +0000, Peter Maydell wrote:
> Use the softfloat conversion routines for conversion to 16 bit
> integers, because just casting to a 16 bit type truncates the
> value rather than saturating it at 16-bit MAXINT/MININT.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Nathan Froyd <froydnj@codesourcery.com>

-Nathan

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Peter Maydell]

On 29 November 2010 17:49, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Tue, Nov 23, 2010 at 06:53:47PM +0000, Peter Maydell wrote:
>> +    /* ARM requires that S<->D conversion of any kind of NaN generates
>> +     * a quiet NaN by forcing the most significant frac bit to 1.
>> +     */
>> +    if (float64_is_signaling_nan(r)) {
>> +        return make_float64(float64_val(r) | (1LL << 51));
>> +    }

> As with other NaN-handling patches, I don't think the bit-twiddling here
> is a good idea.  Having a float*_maybe_silence_nan function in softfloat
> would be a better approach.

I guess this (like the other one you commented on) boils down to how
you want to approach the boundary between qemu proper and the
softfloat library. There are three approaches I can see:

(a) live with the softfloat API as it is, and add bit twiddling as
necessary for particular target CPU special casing in the per-cpu
functions (which is what I was doing here and with the 'is it a NaN?'
function in the other patch)
(b) add to and extend the softfloat API whenever you have some
floating-point related thing it doesn't currently support (which I
did with the "add conversions to int16_t" patch because it was
a big chunk of bit twiddling, but which I felt was a bit invasive to
do for this sort of minor tweak, especially since softfloat is a
copy of a third-party library)
(c) do something suboptimal where the softfloat API provides
some-API-but-not-quite-the-ideal-API (which I'm not particularly
keen on and is what I see the "is_nan() || is_signalling_nan()"
approach as)

My original patchset tends to (a) except where (b) is clearly
more sensible; if people would prefer (b) all the time I'm happy
to do things that way; (c) doesn't seem very attractive to me
and I would rather do (b) in those situations.

-- PMM

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Nathan Froyd]

On Mon, Nov 29, 2010 at 07:25:18PM +0000, Peter Maydell wrote:
> On 29 November 2010 17:49, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > On Tue, Nov 23, 2010 at 06:53:47PM +0000, Peter Maydell wrote:
> > As with other NaN-handling patches, I don't think the bit-twiddling here
> > is a good idea.  Having a float*_maybe_silence_nan function in softfloat
> > would be a better approach.
> 
> I guess this (like the other one you commented on) boils down to how
> you want to approach the boundary between qemu proper and the
> softfloat library. There are three approaches I can see:
> 
> (a) live with the softfloat API as it is, and add bit twiddling as
> necessary for particular target CPU special casing in the per-cpu
> functions (which is what I was doing here and with the 'is it a NaN?'
> function in the other patch)

Full disclosure: I did this sort of thing for PPC; see the DO_HANDLE_NAN
macro in op_helper.c.  I was young and thoughtless then and now I
am...well, older, anyway. :) I don't think it's the best approach: since
at least two CPUs now need NaN-silencing, we should provide something
generic.  And even if only one CPU requires it, I think it's better to
squirrel the logic away in fpu/.  float{32,64,80,128} should be opaque
things except for specialized cases.

(I can see an argument for CPU-confined bit twiddling to implement
things like float*_rsqrt_estimate or similar, where one function might
not work across CPU families due to precision requirements and so forth.
But we can cross that bridge when we come to it.)

> (b) add to and extend the softfloat API whenever you have some
> floating-point related thing it doesn't currently support (which I
> did with the "add conversions to int16_t" patch because it was
> a big chunk of bit twiddling, but which I felt was a bit invasive to
> do for this sort of minor tweak, especially since softfloat is a
> copy of a third-party library)

I think this is the best approach whenever possible.  I would not be too
worried about the third-party-ness of softfloat; it's extremely stable,
unlikely to change anytime in the near or far future, and we've already
extended in it non-trivial ways anyway.  (And would do so again if we
ever implemented, say, proper flush-to-zero denormal handling or IA64
register-precision floats--the former being more likely than the
latter. ;)

An example of where softfloat could be usefully extended and where we do
(a) sometimes is in production of CPU-default NaN values.  softfloat
knows all about this, yet there are #defines scattered about to provide
bit patterns because the softfloat API isn't extensive enough.

> (c) do something suboptimal where the softfloat API provides
> some-API-but-not-quite-the-ideal-API (which I'm not particularly
> keen on and is what I see the "is_nan() || is_signalling_nan()"
> approach as)

Yes, this is ugly.  Are you up for running:

  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c

(and also carefully in fpu/*) or similar and moving the bit-twiddling
float_is_nan into fpu/?

-Nathan

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Peter Maydell]

On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Mon, Nov 29, 2010 at 07:25:18PM +0000, Peter Maydell wrote:
>> (b) add to and extend the softfloat API whenever you have some
>> floating-point related thing it doesn't currently support
>
> I think this is the best approach whenever possible.  I would not be too
> worried about the third-party-ness of softfloat; it's extremely stable,
> unlikely to change anytime in the near or far future, and we've already
> extended in it non-trivial ways anyway.

OK. Do we care about maintaining consistency of the API between
softfloat and softfloat-native (the latter used only on x86, x86_64,
cris, sh4, sh4eb)? I didn't in these patchsets because there didn't
seem any point adding functions to softfloat-native that weren't
going to be used by anything and so couldn't be tested.

>> (c) do something suboptimal where the softfloat API provides
>> some-API-but-not-quite-the-ideal-API (which I'm not particularly
>> keen on and is what I see the "is_nan() || is_signalling_nan()"
>> approach as)
>
> Yes, this is ugly.  Are you up for running:
>
>  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
>
> (and also carefully in fpu/*) or similar and moving the bit-twiddling
> float_is_nan into fpu/?

I'm happy to produce a patch doing that if it will be committed :-)

-- PMM

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Nathan Froyd]

On Mon, Nov 29, 2010 at 08:04:42PM +0000, Peter Maydell wrote:
> On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > On Mon, Nov 29, 2010 at 07:25:18PM +0000, Peter Maydell wrote:
> >> (b) add to and extend the softfloat API whenever you have some
> >> floating-point related thing it doesn't currently support
> >
> > I think this is the best approach whenever possible.
> 
> OK. Do we care about maintaining consistency of the API between
> softfloat and softfloat-native (the latter used only on x86, x86_64,
> cris, sh4, sh4eb)?

softfloat-native should just go away.  I would not worry about API
compatibility between native and non-native configurations there.

> >> (c) do something suboptimal where the softfloat API provides
> >> some-API-but-not-quite-the-ideal-API (which I'm not particularly
> >> keen on and is what I see the "is_nan() || is_signalling_nan()"
> >> approach as)
> >
> > Yes, this is ugly.  Are you up for running:
> >
> >  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
> >
> > (and also carefully in fpu/*) or similar and moving the bit-twiddling
> > float_is_nan into fpu/?
> 
> I'm happy to produce a patch doing that if it will be committed :-)

Well, I can't promise the committal part... :)

-Nathan

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Peter Maydell]

On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> Yes, this is ugly.  Are you up for running:
>
>  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
>
> (and also carefully in fpu/*) or similar and moving the bit-twiddling
> float_is_nan into fpu/?

I'm just compiling this up now. While I was eyeballing the results of
the substitution, I noticed that there are some places which are almost
certainly bugs introduced by other people not noticing that float*_is_nan()
doesn't do what it says on the tin. Three at random:

In target-ppc/op_helper.c:helper_compute_fprf():
    if (unlikely(float64_is_nan(farg.d))) {
        if (float64_is_signaling_nan(farg.d)) {
            /* Signaling NaN: flags are undefined */
            ret = 0x00;
        } else {
            /* Quiet NaN */
            ret = 0x11;
        }
    } else ...

the 'signalling NaN' case can never be taken.

In target-alpha/op_helper.c:helper_cmptun():
    if (float64_is_nan(fa) || float64_is_nan(fb))
        return 0x4000000000000000ULL;
    else
        return 0;

...but the web suggests this is supposed to return non-zero for any
unordered comparison, not just ones with quiet NaNs.

In target-m68k/helper.c:sub_cmp_f64:
    res = float64_sub(a, b, &env->fp_status);
    if (float64_is_nan(res)) {
        /* +/-inf compares equal against itself, but sub returns nan.  */
        if (!float64_is_nan(a)
            && !float64_is_nan(b)) {
            res = float64_zero;
            if (float64_lt_quiet(a, res, &env->fp_status))
                res = float64_chs(res);
        }
    }

judging from the comments the author expected is_nan() to
be true for all NaNs.

I'm not sure what we should do about these -- they look wrong
but I don't have any of the setup to actually test whether they're
wrong. (The only ARM ones are in the Linux user nwfpe emulation
which is pretty much dead by now IMHO.)

-- PMM

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Nathan Froyd]

On Tue, Nov 30, 2010 at 11:15:56AM +0000, Peter Maydell wrote:
> On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > Yes, this is ugly.  Are you up for running:
> >
> >  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
> >
> > (and also carefully in fpu/*) or similar and moving the bit-twiddling
> > float_is_nan into fpu/?
> 
> I'm just compiling this up now. While I was eyeballing the results of
> the substitution, I noticed that there are some places which are almost
> certainly bugs introduced by other people not noticing that float*_is_nan()
> doesn't do what it says on the tin. Three at random:
> 
> In target-ppc/op_helper.c:helper_compute_fprf():
> 
> In target-alpha/op_helper.c:helper_cmptun():
> 
> In target-m68k/helper.c:sub_cmp_f64:
> 
> judging from the comments the author expected is_nan() to
> be true for all NaNs.
> 
> I'm not sure what we should do about these -- they look wrong
> but I don't have any of the setup to actually test whether they're
> wrong.

RTH (CC'd) is the expert on the Alpha bits.  The PPC one is obviously
wrong.  I can test the m68k one.

In any event, I think the safest course is to do the simple renaming; we
can clean up broken bits after the fact.

-Nathan

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Richard Henderson]

On 12/01/2010 07:39 AM, Nathan Froyd wrote:
> RTH (CC'd) is the expert on the Alpha bits.

The Alpha cmptun is supposed to return true for both Q+SNaN.

Although, Invalid Operand is supposed to be raised for SNaN,
which is not happening here in helper_cmptun.  Or, indeed,
any of the comparison helper functions.

I think I've lost the thread a bit -- is the proposal to
replace the existing float*_is_nan with _is_quiet_nan and
invent a new function that returns true for both Q+S?  That
at least would be monotonic improvement for Alpha, although
as noted above not 100% correct.


r~

Re: [Qemu-devel] [PATCH 08/12] ARM: Return correct result for single<->double conversion of NaN

[Nathan Froyd]

On Wed, Dec 01, 2010 at 09:52:13AM -0800, Richard Henderson wrote:
> I think I've lost the thread a bit -- is the proposal to
> replace the existing float*_is_nan with _is_quiet_nan and
> invent a new function that returns true for both Q+S?  That
> at least would be monotonic improvement for Alpha, although
> as noted above not 100% correct.

Yes, that's the plan.  As (planned to be) implemented, doing this should
not change anything:

1. s/float*_is_nan/float*_is_quiet_nan/g
2. write new float*_is_nan
3. use new float*_is_nan in new code
4. replace bogus float*_is_quiet_nan uses with new function

-Nathan