| Message ID | 20201022120830.5938-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Drop unused VMSTATE_FLOAT64 support | expand |
* 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 >
* 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 >
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(-)
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
+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 >
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
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,
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(-)