| Message ID | 1301915362-2626-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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 > > >
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;
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
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
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
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;
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(-)