[Qemu-devel] [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Qemu-devel] [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Peter Maydell]

CPU_QuadU isn't used on all targets, but there's no harm in defining the
typedef anyway. It only needs to be guarded by CONFIG_SOFTFLOAT, because
softfloat-native doesn't have a float128 type. This avoids the need for
every new target which uses CPU_QuadU to add itself to an #ifdef in
what ought to be target-agnostic code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Noticed this one as a result of the s390 support patches; seems like
a minor but worthwhile cleanup to me...

 cpu-all.h |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index 4cc445f..dc0f2f0 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -138,11 +138,10 @@ typedef union {
     uint64_t ll;
 } CPU_DoubleU;
 
-#if defined(TARGET_SPARC) || defined(TARGET_S390X)
+#if defined(CONFIG_SOFTFLOAT)
 typedef union {
     float128 q;
-#if defined(HOST_WORDS_BIGENDIAN) \
-    || (defined(__arm__) && !defined(__VFP_FP__) && !defined(CONFIG_SOFTFLOAT))
+#if defined(HOST_WORDS_BIGENDIAN)
     struct {
         uint32_t upmost;
         uint32_t upper;
-- 
1.7.1

[Qemu-devel] Re: [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Alexander Graf]

On 04/04/2011 01:09 PM, Peter Maydell wrote:
> CPU_QuadU isn't used on all targets, but there's no harm in defining the
> typedef anyway. It only needs to be guarded by CONFIG_SOFTFLOAT, because
> softfloat-native doesn't have a float128 type. This avoids the need for
> every new target which uses CPU_QuadU to add itself to an #ifdef in
> what ought to be target-agnostic code.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>

I don't really know my way around FP, but from here it looks good :). 
Not sure about the arm part, but I trust Peter on that one ;).


Alex

[Qemu-devel] Re: [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Peter Maydell]

On 4 April 2011 15:47, Alexander Graf <agraf@suse.de> wrote:
> On 04/04/2011 01:09 PM, Peter Maydell wrote:
>>
>> CPU_QuadU isn't used on all targets, but there's no harm in defining the
>> typedef anyway. It only needs to be guarded by CONFIG_SOFTFLOAT, because
>> softfloat-native doesn't have a float128 type. This avoids the need for
>> every new target which uses CPU_QuadU to add itself to an #ifdef in
>> what ought to be target-agnostic code.
>>
>> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
>
> I don't really know my way around FP, but from here it looks good :). Not
> sure about the arm part, but I trust Peter on that one ;).

The __arm__ part of the ifdef was a voodoo-copy from the CPU_DoubleU
typedef, where it actually does matter if you're building a softfloat-native
target on an ARM host which uses the ancient FPA floating point ABI
(as the comment says, although ARM is generally little-endian doubles
are stored in memory in big-endian order under FPA; the more modern
VFP has them little-endian). This ifdef condition was meaningless for
CPU_QuadU (because FPA didn't have a 128 bit native float type for the
type to try to be compatible with) and could never have kicked in
anyhow because we wouldn't have compiled unless CONFIG_SOFTFLOAT
was defined.

-- PMM

Re: [Qemu-devel] [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Aurelien Jarno]

On Mon, Apr 04, 2011 at 12:09:22PM +0100, Peter Maydell wrote:
> CPU_QuadU isn't used on all targets, but there's no harm in defining the
> typedef anyway. It only needs to be guarded by CONFIG_SOFTFLOAT, because
> softfloat-native doesn't have a float128 type. This avoids the need for
> every new target which uses CPU_QuadU to add itself to an #ifdef in
> what ought to be target-agnostic code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Noticed this one as a result of the s390 support patches; seems like
> a minor but worthwhile cleanup to me...
> 
>  cpu-all.h |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)

Thanks, applied.

> diff --git a/cpu-all.h b/cpu-all.h
> index 4cc445f..dc0f2f0 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -138,11 +138,10 @@ typedef union {
>      uint64_t ll;
>  } CPU_DoubleU;
>  
> -#if defined(TARGET_SPARC) || defined(TARGET_S390X)
> +#if defined(CONFIG_SOFTFLOAT)
>  typedef union {
>      float128 q;
> -#if defined(HOST_WORDS_BIGENDIAN) \
> -    || (defined(__arm__) && !defined(__VFP_FP__) && !defined(CONFIG_SOFTFLOAT))
> +#if defined(HOST_WORDS_BIGENDIAN)
>      struct {
>          uint32_t upmost;
>          uint32_t upper;
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Laurent Vivier]

Le lundi 04 avril 2011 à 12:09 +0100, Peter Maydell a écrit :
> CPU_QuadU isn't used on all targets, but there's no harm in defining the
> typedef anyway. It only needs to be guarded by CONFIG_SOFTFLOAT, because
> softfloat-native doesn't have a float128 type. This avoids the need for
> every new target which uses CPU_QuadU to add itself to an #ifdef in
> what ought to be target-agnostic code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Noticed this one as a result of the s390 support patches; seems like
> a minor but worthwhile cleanup to me...

Sorry for the late comment, but I saw this rebasing my patchset on
master...

>  cpu-all.h |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 4cc445f..dc0f2f0 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -138,11 +138,10 @@ typedef union {
>      uint64_t ll;
>  } CPU_DoubleU;
>  
> -#if defined(TARGET_SPARC) || defined(TARGET_S390X)
> +#if defined(CONFIG_SOFTFLOAT)

Why don't you use "#if defined(FLOAT128)" ?

>  typedef union {
>      float128 q;
> -#if defined(HOST_WORDS_BIGENDIAN) \
> -    || (defined(__arm__) && !defined(__VFP_FP__) && !defined(CONFIG_SOFTFLOAT))
> +#if defined(HOST_WORDS_BIGENDIAN)
>      struct {
>          uint32_t upmost;
>          uint32_t upper;

-- 
--------------------- laurent@vivier.eu ----------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard

Re: [Qemu-devel] [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Peter Maydell]

On 5 April 2011 22:48, Laurent Vivier <Laurent@vivier.eu> wrote:
> Le lundi 04 avril 2011 à 12:09 +0100, Peter Maydell a écrit :
>> -#if defined(TARGET_SPARC) || defined(TARGET_S390X)
>> +#if defined(CONFIG_SOFTFLOAT)
>
> Why don't you use "#if defined(FLOAT128)" ?

I did consider that, but I felt FLOAT128 was a softfloat-internal
macro rather than part of the API softfloat provides to the rest
of qemu.

-- PMM

Re: [Qemu-devel] [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Laurent Vivier]

Le mardi 05 avril 2011 à 22:54 +0100, Peter Maydell a écrit :
> On 5 April 2011 22:48, Laurent Vivier <Laurent@vivier.eu> wrote:
> > Le lundi 04 avril 2011 à 12:09 +0100, Peter Maydell a écrit :
> >> -#if defined(TARGET_SPARC) || defined(TARGET_S390X)
> >> +#if defined(CONFIG_SOFTFLOAT)
> >
> > Why don't you use "#if defined(FLOAT128)" ?
> 
> I did consider that, but I felt FLOAT128 was a softfloat-internal
> macro rather than part of the API softfloat provides to the rest
> of qemu.

But, for instance, "#ifdef FLOATX80" is used in target-i386/cpu.h and
target-i386/op_helper.c.

Regards,
Laurent
-- 
--------------------- laurent@vivier.eu ----------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard

Re: [Qemu-devel] [PATCH] cpu-all.h: Remove unnecessary target-specific ifdef for CPU_QuadU

[Peter Maydell]

On 5 April 2011 23:15, Laurent Vivier <Laurent@vivier.eu> wrote:
> Le mardi 05 avril 2011 à 22:54 +0100, Peter Maydell a écrit :
>> On 5 April 2011 22:48, Laurent Vivier <Laurent@vivier.eu> wrote:
>> > Le lundi 04 avril 2011 à 12:09 +0100, Peter Maydell a écrit :
>> >> -#if defined(TARGET_SPARC) || defined(TARGET_S390X)
>> >> +#if defined(CONFIG_SOFTFLOAT)
>> >
>> > Why don't you use "#if defined(FLOAT128)" ?
>>
>> I did consider that, but I felt FLOAT128 was a softfloat-internal
>> macro rather than part of the API softfloat provides to the rest
>> of qemu.
>
> But, for instance, "#ifdef FLOATX80" is used in target-i386/cpu.h and
> target-i386/op_helper.c.

Those uses seem conceptually wrong to me: either a target needs
80 bit floats, or it doesn't. It shouldn't behave differently
depending on what host it was compiled on. However this is really
just legacy of the fact that target-i386 is still compiled with
softfloat-native rather than proper softfloat.

Personally I would also delete the FLOAT128 ifdefs in target-ppc
since we always compile with softfloat now.

However I don't feel very strongly about any of this; mostly I
just wanted to get rid of the target and host specific ifdeffery.

-- PMM