[PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[Peter Maydell]

Commit ef96e3ae9698d6 in January 2019 removed the last user of the
VMSTATE_FLOAT64* macros. These were used by targets which defined
their floating point register file as an array of 'float64'.

We used to try to maintain a stricter distinction between
'float64' (a type for holding an integer representing an IEEE float)
and 'uint64_t', including having a debug option for 'float64' being
a struct and supposedly mandatory macros for converting between
float64 and uint64_t. We no longer think that's a usefully
strong distinction to draw and we allow ourselves to freely
assume that float64 really is just a 64-bit integer type, so
for new targets we would simply recommend use of the uint64_t type
for a floating point register file. The float64 type remains
as a useful way of documenting in the type signature of helper
functions and the like that they expect to receive an IEEE float
from the TCG generated code rather than an arbitrary integer.

Since the VMSTATE_FLOAT64* macros have no remaining users and
we don't recommend new code uses them, delete them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/migration/vmstate.h | 13 -------------
 migration/vmstate-types.c   | 26 --------------------------
 2 files changed, 39 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index f68ed7db13c..4d71dc8fbaa 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -219,7 +219,6 @@ extern const VMStateInfo vmstate_info_uint64;
 #define VMS_NULLPTR_MARKER (0x30U) /* '0' */
 extern const VMStateInfo vmstate_info_nullptr;
 
-extern const VMStateInfo vmstate_info_float64;
 extern const VMStateInfo vmstate_info_cpudouble;
 
 extern const VMStateInfo vmstate_info_timer;
@@ -997,12 +996,6 @@ extern const VMStateInfo vmstate_info_qlist;
     VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_uint64, uint64_t)
 
 
-#define VMSTATE_FLOAT64_V(_f, _s, _v)                                 \
-    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_float64, float64)
-
-#define VMSTATE_FLOAT64(_f, _s)                                       \
-    VMSTATE_FLOAT64_V(_f, _s, 0)
-
 #define VMSTATE_TIMER_PTR_TEST(_f, _s, _test)                             \
     VMSTATE_POINTER_TEST(_f, _s, _test, vmstate_info_timer, QEMUTimer *)
 
@@ -1114,12 +1107,6 @@ extern const VMStateInfo vmstate_info_qlist;
 #define VMSTATE_INT64_ARRAY(_f, _s, _n)                               \
     VMSTATE_INT64_ARRAY_V(_f, _s, _n, 0)
 
-#define VMSTATE_FLOAT64_ARRAY_V(_f, _s, _n, _v)                       \
-    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_float64, float64)
-
-#define VMSTATE_FLOAT64_ARRAY(_f, _s, _n)                             \
-    VMSTATE_FLOAT64_ARRAY_V(_f, _s, _n, 0)
-
 #define VMSTATE_CPUDOUBLE_ARRAY_V(_f, _s, _n, _v)                     \
     VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_cpudouble, CPU_DoubleU)
 
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 35e784c9d93..e22d41d73d6 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -420,32 +420,6 @@ const VMStateInfo vmstate_info_uint16_equal = {
     .put  = put_uint16,
 };
 
-/* floating point */
-
-static int get_float64(QEMUFile *f, void *pv, size_t size,
-                       const VMStateField *field)
-{
-    float64 *v = pv;
-
-    *v = make_float64(qemu_get_be64(f));
-    return 0;
-}
-
-static int put_float64(QEMUFile *f, void *pv, size_t size,
-                       const VMStateField *field, QJSON *vmdesc)
-{
-    uint64_t *v = pv;
-
-    qemu_put_be64(f, float64_val(*v));
-    return 0;
-}
-
-const VMStateInfo vmstate_info_float64 = {
-    .name = "float64",
-    .get  = get_float64,
-    .put  = put_float64,
-};
-
 /* CPU_DoubleU type */
 
 static int get_cpudouble(QEMUFile *f, void *pv, size_t size,
-- 
2.20.1

Re: [PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Commit ef96e3ae9698d6 in January 2019 removed the last user of the
> VMSTATE_FLOAT64* macros. These were used by targets which defined
> their floating point register file as an array of 'float64'.
> 
> We used to try to maintain a stricter distinction between
> 'float64' (a type for holding an integer representing an IEEE float)
> and 'uint64_t', including having a debug option for 'float64' being
> a struct and supposedly mandatory macros for converting between
> float64 and uint64_t. We no longer think that's a usefully
> strong distinction to draw and we allow ourselves to freely
> assume that float64 really is just a 64-bit integer type, so
> for new targets we would simply recommend use of the uint64_t type
> for a floating point register file. The float64 type remains
> as a useful way of documenting in the type signature of helper
> functions and the like that they expect to receive an IEEE float
> from the TCG generated code rather than an arbitrary integer.
> 
> Since the VMSTATE_FLOAT64* macros have no remaining users and
> we don't recommend new code uses them, delete them.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/migration/vmstate.h | 13 -------------
>  migration/vmstate-types.c   | 26 --------------------------
>  2 files changed, 39 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f68ed7db13c..4d71dc8fbaa 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -219,7 +219,6 @@ extern const VMStateInfo vmstate_info_uint64;
>  #define VMS_NULLPTR_MARKER (0x30U) /* '0' */
>  extern const VMStateInfo vmstate_info_nullptr;
>  
> -extern const VMStateInfo vmstate_info_float64;
>  extern const VMStateInfo vmstate_info_cpudouble;
>  
>  extern const VMStateInfo vmstate_info_timer;
> @@ -997,12 +996,6 @@ extern const VMStateInfo vmstate_info_qlist;
>      VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_uint64, uint64_t)
>  
>  
> -#define VMSTATE_FLOAT64_V(_f, _s, _v)                                 \
> -    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_float64, float64)
> -
> -#define VMSTATE_FLOAT64(_f, _s)                                       \
> -    VMSTATE_FLOAT64_V(_f, _s, 0)
> -
>  #define VMSTATE_TIMER_PTR_TEST(_f, _s, _test)                             \
>      VMSTATE_POINTER_TEST(_f, _s, _test, vmstate_info_timer, QEMUTimer *)
>  
> @@ -1114,12 +1107,6 @@ extern const VMStateInfo vmstate_info_qlist;
>  #define VMSTATE_INT64_ARRAY(_f, _s, _n)                               \
>      VMSTATE_INT64_ARRAY_V(_f, _s, _n, 0)
>  
> -#define VMSTATE_FLOAT64_ARRAY_V(_f, _s, _n, _v)                       \
> -    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_float64, float64)
> -
> -#define VMSTATE_FLOAT64_ARRAY(_f, _s, _n)                             \
> -    VMSTATE_FLOAT64_ARRAY_V(_f, _s, _n, 0)
> -
>  #define VMSTATE_CPUDOUBLE_ARRAY_V(_f, _s, _n, _v)                     \
>      VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_cpudouble, CPU_DoubleU)
>  
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 35e784c9d93..e22d41d73d6 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -420,32 +420,6 @@ const VMStateInfo vmstate_info_uint16_equal = {
>      .put  = put_uint16,
>  };
>  
> -/* floating point */
> -
> -static int get_float64(QEMUFile *f, void *pv, size_t size,
> -                       const VMStateField *field)
> -{
> -    float64 *v = pv;
> -
> -    *v = make_float64(qemu_get_be64(f));
> -    return 0;
> -}
> -
> -static int put_float64(QEMUFile *f, void *pv, size_t size,
> -                       const VMStateField *field, QJSON *vmdesc)
> -{
> -    uint64_t *v = pv;
> -
> -    qemu_put_be64(f, float64_val(*v));
> -    return 0;
> -}
> -
> -const VMStateInfo vmstate_info_float64 = {
> -    .name = "float64",
> -    .get  = get_float64,
> -    .put  = put_float64,
> -};
> -
>  /* CPU_DoubleU type */
>  
>  static int get_cpudouble(QEMUFile *f, void *pv, size_t size,
> -- 
> 2.20.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Commit ef96e3ae9698d6 in January 2019 removed the last user of the
> VMSTATE_FLOAT64* macros. These were used by targets which defined
> their floating point register file as an array of 'float64'.
> 
> We used to try to maintain a stricter distinction between
> 'float64' (a type for holding an integer representing an IEEE float)
> and 'uint64_t', including having a debug option for 'float64' being
> a struct and supposedly mandatory macros for converting between
> float64 and uint64_t. We no longer think that's a usefully
> strong distinction to draw and we allow ourselves to freely
> assume that float64 really is just a 64-bit integer type, so
> for new targets we would simply recommend use of the uint64_t type
> for a floating point register file. The float64 type remains
> as a useful way of documenting in the type signature of helper
> functions and the like that they expect to receive an IEEE float
> from the TCG generated code rather than an arbitrary integer.
> 
> Since the VMSTATE_FLOAT64* macros have no remaining users and
> we don't recommend new code uses them, delete them.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Queued

> ---
>  include/migration/vmstate.h | 13 -------------
>  migration/vmstate-types.c   | 26 --------------------------
>  2 files changed, 39 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f68ed7db13c..4d71dc8fbaa 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -219,7 +219,6 @@ extern const VMStateInfo vmstate_info_uint64;
>  #define VMS_NULLPTR_MARKER (0x30U) /* '0' */
>  extern const VMStateInfo vmstate_info_nullptr;
>  
> -extern const VMStateInfo vmstate_info_float64;
>  extern const VMStateInfo vmstate_info_cpudouble;
>  
>  extern const VMStateInfo vmstate_info_timer;
> @@ -997,12 +996,6 @@ extern const VMStateInfo vmstate_info_qlist;
>      VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_uint64, uint64_t)
>  
>  
> -#define VMSTATE_FLOAT64_V(_f, _s, _v)                                 \
> -    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_float64, float64)
> -
> -#define VMSTATE_FLOAT64(_f, _s)                                       \
> -    VMSTATE_FLOAT64_V(_f, _s, 0)
> -
>  #define VMSTATE_TIMER_PTR_TEST(_f, _s, _test)                             \
>      VMSTATE_POINTER_TEST(_f, _s, _test, vmstate_info_timer, QEMUTimer *)
>  
> @@ -1114,12 +1107,6 @@ extern const VMStateInfo vmstate_info_qlist;
>  #define VMSTATE_INT64_ARRAY(_f, _s, _n)                               \
>      VMSTATE_INT64_ARRAY_V(_f, _s, _n, 0)
>  
> -#define VMSTATE_FLOAT64_ARRAY_V(_f, _s, _n, _v)                       \
> -    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_float64, float64)
> -
> -#define VMSTATE_FLOAT64_ARRAY(_f, _s, _n)                             \
> -    VMSTATE_FLOAT64_ARRAY_V(_f, _s, _n, 0)
> -
>  #define VMSTATE_CPUDOUBLE_ARRAY_V(_f, _s, _n, _v)                     \
>      VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_cpudouble, CPU_DoubleU)
>  
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 35e784c9d93..e22d41d73d6 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -420,32 +420,6 @@ const VMStateInfo vmstate_info_uint16_equal = {
>      .put  = put_uint16,
>  };
>  
> -/* floating point */
> -
> -static int get_float64(QEMUFile *f, void *pv, size_t size,
> -                       const VMStateField *field)
> -{
> -    float64 *v = pv;
> -
> -    *v = make_float64(qemu_get_be64(f));
> -    return 0;
> -}
> -
> -static int put_float64(QEMUFile *f, void *pv, size_t size,
> -                       const VMStateField *field, QJSON *vmdesc)
> -{
> -    uint64_t *v = pv;
> -
> -    qemu_put_be64(f, float64_val(*v));
> -    return 0;
> -}
> -
> -const VMStateInfo vmstate_info_float64 = {
> -    .name = "float64",
> -    .get  = get_float64,
> -    .put  = put_float64,
> -};
> -
>  /* CPU_DoubleU type */
>  
>  static int get_cpudouble(QEMUFile *f, void *pv, size_t size,
> -- 
> 2.20.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[Philippe Mathieu-Daudé]

On 10/22/20 2:08 PM, Peter Maydell wrote:
> Commit ef96e3ae9698d6 in January 2019 removed the last user of the
> VMSTATE_FLOAT64* macros. These were used by targets which defined
> their floating point register file as an array of 'float64'.

Similar candidate: VMSTATE_CPUDOUBLE_ARRAY()

> We used to try to maintain a stricter distinction between
> 'float64' (a type for holding an integer representing an IEEE float)
> and 'uint64_t', including having a debug option for 'float64' being
> a struct and supposedly mandatory macros for converting between
> float64 and uint64_t. We no longer think that's a usefully
> strong distinction to draw and we allow ourselves to freely
> assume that float64 really is just a 64-bit integer type, so
> for new targets we would simply recommend use of the uint64_t type
> for a floating point register file. The float64 type remains
> as a useful way of documenting in the type signature of helper
> functions and the like that they expect to receive an IEEE float
> from the TCG generated code rather than an arbitrary integer.
> 
> Since the VMSTATE_FLOAT64* macros have no remaining users and
> we don't recommend new code uses them, delete them.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/migration/vmstate.h | 13 -------------
>  migration/vmstate-types.c   | 26 --------------------------
>  2 files changed, 39 deletions(-)

Re: [PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[Peter Maydell]

On Sun, 7 Feb 2021 at 17:10, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 10/22/20 2:08 PM, Peter Maydell wrote:
> > Commit ef96e3ae9698d6 in January 2019 removed the last user of the
> > VMSTATE_FLOAT64* macros. These were used by targets which defined
> > their floating point register file as an array of 'float64'.
>
> Similar candidate: VMSTATE_CPUDOUBLE_ARRAY()

Isn't that still used by target/sparc ?

-- PMM

Re: [PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[Philippe Mathieu-Daudé]

+Eduardo/Richard.

On 2/7/21 8:43 PM, Peter Maydell wrote:
> On Sun, 7 Feb 2021 at 17:10, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 10/22/20 2:08 PM, Peter Maydell wrote:
>>> Commit ef96e3ae9698d6 in January 2019 removed the last user of the
>>> VMSTATE_FLOAT64* macros. These were used by targets which defined
>>> their floating point register file as an array of 'float64'.
>>
>> Similar candidate: VMSTATE_CPUDOUBLE_ARRAY()
> 
> Isn't that still used by target/sparc ?

Sorry, I meant CPU_DoubleU could be a similar to the removal of float64
in commit ef96e3ae969 ("target/ppc: move FP and VMX registers into
aligned vsr register array"), so we can remove VMSTATE_CPUDOUBLE_ARRAY()
later.

But I could be wrong and this is a legit use, as CPU_DoubleU contains
either a float64 or a uint64_t.

Now if the CPU_DoubleU/CPU_QuadU unions are only meant for CPU
registers, we should either document that or better move these
declarations out of the generic "qemu/bswap.h" ("exec/cpu-defs.h"
looks like a good candidate).

> 
> -- PMM
> 

Re: [PATCH] migration: Drop unused VMSTATE_FLOAT64 support

[Peter Maydell]

On Mon, 8 Feb 2021 at 09:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Eduardo/Richard.
>
> On 2/7/21 8:43 PM, Peter Maydell wrote:
> > On Sun, 7 Feb 2021 at 17:10, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> On 10/22/20 2:08 PM, Peter Maydell wrote:
> >>> Commit ef96e3ae9698d6 in January 2019 removed the last user of the
> >>> VMSTATE_FLOAT64* macros. These were used by targets which defined
> >>> their floating point register file as an array of 'float64'.
> >>
> >> Similar candidate: VMSTATE_CPUDOUBLE_ARRAY()
> >
> > Isn't that still used by target/sparc ?
>
> Sorry, I meant CPU_DoubleU could be a similar to the removal of float64
> in commit ef96e3ae969 ("target/ppc: move FP and VMX registers into
> aligned vsr register array"), so we can remove VMSTATE_CPUDOUBLE_ARRAY()
> later.
>
> But I could be wrong and this is a legit use, as CPU_DoubleU contains
> either a float64 or a uint64_t.

CPU_DoubleU and friends are there to provide host-endian-independent
access to the different parts of a larger-than-word-size value (which is
why they're defined in bswap.h). They happen to use 'float64 d' as one
of the union subtypes as well as 'uint64_t ll', because they pre-date
the decision that we shouldn't really care about the distinction
between float64 and uint64_t except for documentation purposes in
helper function prototypes. But the core reason they're present is
to provide conversion between 'd' or 'll' and ('upper', 'lower').

There is some cleanup that could be done of these types now that we
don't try to distinguish float64 and uint64_t:

(1) CPU_FloatU is now entirely unnecessary, and uses like this (alpha):

static inline uint64_t float32_to_s(float32 fa)
{
    CPU_FloatU a;
    a.f = fa;
    return float32_to_s_int(a.l);
}

could just be written:

static inline uint64_t float32_to_s(float32 fa)
{
    return float32_to_s_int(fa);
}

(and then rename float32_to_s_int() to float32_to_s() and drop
the wrapper entirely)

(2) Where CPU_DoubleU is being used only for transitions between 'd'
and 'll', like this (ppc):

uint64_t helper_frsp(CPUPPCState *env, uint64_t arg)
{
    CPU_DoubleU farg;
    float32 f32;

    farg.ll = arg;

    if (unlikely(float64_is_signaling_nan(farg.d, &env->fp_status))) {
        float_invalid_op_vxsnan(env, GETPC());
    }
    f32 = float64_to_float32(farg.d, &env->fp_status);
    farg.d = float32_to_float64(f32, &env->fp_status);

    return farg.ll;
}

we can drop the use of CPU_DoubleU and just pass 'arg'
directly where 'farg.d' was being passed, and similarly
just return the result of float32_to_float64() without
passing it through the union.

But at least some uses of these types will remain after that, I think.
(We could look again at what those remaining uses are after the
first round of cleanup and whether there are better ways to write
that code; personally I find these unions a bit ugly and wouldn't be
sorry to see them go away.)

thanks
-- PMM