| Message ID | 20160809190229.27871-2-bobby.prani@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2016-08-09 15:02, Pranith Kumar wrote: > Change the flag type to 'int' to fix the implicit conversion error. > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > --- > fpu/softfloat-specialize.h | 2 +- > include/fpu/softfloat.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h > index 43d0890..46b4091 100644 > --- a/fpu/softfloat-specialize.h > +++ b/fpu/softfloat-specialize.h > @@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status) > | should be simply `float_exception_flags |= flags;'. > *----------------------------------------------------------------------------*/ > > -void float_raise(int8_t flags, float_status *status) > +void float_raise(int flags, float_status *status) > { > status->float_exception_flags |= flags; > } > diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h > index 0e57ee5..416cf7a 100644 > --- a/include/fpu/softfloat.h > +++ b/include/fpu/softfloat.h > @@ -196,9 +196,9 @@ enum { > }; > > typedef struct float_status { > + int float_exception_flags; > signed char float_detect_tininess; > signed char float_rounding_mode; > - signed char float_exception_flags; > signed char floatx80_rounding_precision; > /* should denormalised results go to zero and set the inexact flag? */ > flag flush_to_zero; This changes the size of the structure, and thus of the CPU*State structures. I don't think it's something we want to do, especially given we currently only use 7 flags, so 7 bits and that fits in a char.
On 9 August 2016 at 20:16, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2016-08-09 15:02, Pranith Kumar wrote: >> Change the flag type to 'int' to fix the implicit conversion error. >> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> --- >> fpu/softfloat-specialize.h | 2 +- >> include/fpu/softfloat.h | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h >> index 43d0890..46b4091 100644 >> --- a/fpu/softfloat-specialize.h >> +++ b/fpu/softfloat-specialize.h >> @@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status) >> | should be simply `float_exception_flags |= flags;'. >> *----------------------------------------------------------------------------*/ >> >> -void float_raise(int8_t flags, float_status *status) >> +void float_raise(int flags, float_status *status) >> { >> status->float_exception_flags |= flags; >> } >> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h >> index 0e57ee5..416cf7a 100644 >> --- a/include/fpu/softfloat.h >> +++ b/include/fpu/softfloat.h >> @@ -196,9 +196,9 @@ enum { >> }; >> >> typedef struct float_status { >> + int float_exception_flags; >> signed char float_detect_tininess; >> signed char float_rounding_mode; >> - signed char float_exception_flags; >> signed char floatx80_rounding_precision; >> /* should denormalised results go to zero and set the inexact flag? */ >> flag flush_to_zero; > > This changes the size of the structure, and thus of the CPU*State > structures. I don't think it's something we want to do, especially given > we currently only use 7 flags, so 7 bits and that fits in a char. It does, but only by four bytes, which I didn't think was that big a deal. If we want to keep it to one byte then I think making it a uint8_t is probably better than leaving it as signed char, given we're definitely not treating it as a signed value. thanks -- PMM
On 08/10/2016 12:32 AM, Pranith Kumar wrote: > typedef struct float_status { > + int float_exception_flags; > signed char float_detect_tininess; > signed char float_rounding_mode; > - signed char float_exception_flags; Given that there are no flags outside 8 bits, why is this supposed to help? r~
On 10 August 2016 at 11:32, Richard Henderson <rth@twiddle.net> wrote: > On 08/10/2016 12:32 AM, Pranith Kumar wrote: >> >> typedef struct float_status { >> + int float_exception_flags; >> signed char float_detect_tininess; >> signed char float_rounding_mode; >> - signed char float_exception_flags; > > > Given that there are no flags outside 8 bits, why is this supposed to help? It's just consistency -- using the same type all the way through from the set_float_exception_flags() prototype to the field in the structure. When we were discussing this on IRC that seemed a reasonable approach to me. What clang is specifically warning about is if you pass float_flag_output_denormal to set_float_exception_flags(): that's a positive number (128), which gets implicitly converted to a negative number when it's assigned to the signed char. Using a type wide enough to store 128 all the way through fixes this. (Unsigned char would work too.) thanks -- PMM
On 10/08/2016 12:37, Peter Maydell wrote: > On 10 August 2016 at 11:32, Richard Henderson <rth@twiddle.net> wrote: >> On 08/10/2016 12:32 AM, Pranith Kumar wrote: >>> >>> typedef struct float_status { >>> + int float_exception_flags; >>> signed char float_detect_tininess; >>> signed char float_rounding_mode; >>> - signed char float_exception_flags; >> >> >> Given that there are no flags outside 8 bits, why is this supposed to help? > > It's just consistency -- using the same type all the way through > from the set_float_exception_flags() prototype to the field > in the structure. When we were discussing this on IRC that > seemed a reasonable approach to me. > > What clang is specifically warning about is if you pass > float_flag_output_denormal to set_float_exception_flags(): > that's a positive number (128), which gets implicitly > converted to a negative number when it's assigned to > the signed char. Using a type wide enough to store > 128 all the way through fixes this. (Unsigned char would > work too.) That seems better. By the way this use of "signed char" is a real bug; get_float_exception_flags is returning a sign-extended value, which is turned into a positive value here: if(nRc == 1 && get_float_exception_flags(&fpa11->fp_status)) { //printf("fef 0x%x\n",float_exception_flags); nRc = -get_float_exception_flags(&fpa11->fp_status); } //printf("returning %d\n",nRc); return(nRc); Thanks, Paolo
On 2016-08-09 22:12, Peter Maydell wrote: > On 9 August 2016 at 20:16, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On 2016-08-09 15:02, Pranith Kumar wrote: > >> Change the flag type to 'int' to fix the implicit conversion error. > >> > >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> > >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > >> --- > >> fpu/softfloat-specialize.h | 2 +- > >> include/fpu/softfloat.h | 4 ++-- > >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h > >> index 43d0890..46b4091 100644 > >> --- a/fpu/softfloat-specialize.h > >> +++ b/fpu/softfloat-specialize.h > >> @@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status) > >> | should be simply `float_exception_flags |= flags;'. > >> *----------------------------------------------------------------------------*/ > >> > >> -void float_raise(int8_t flags, float_status *status) > >> +void float_raise(int flags, float_status *status) > >> { > >> status->float_exception_flags |= flags; > >> } > >> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h > >> index 0e57ee5..416cf7a 100644 > >> --- a/include/fpu/softfloat.h > >> +++ b/include/fpu/softfloat.h > >> @@ -196,9 +196,9 @@ enum { > >> }; > >> > >> typedef struct float_status { > >> + int float_exception_flags; > >> signed char float_detect_tininess; > >> signed char float_rounding_mode; > >> - signed char float_exception_flags; > >> signed char floatx80_rounding_precision; > >> /* should denormalised results go to zero and set the inexact flag? */ > >> flag flush_to_zero; > > > > This changes the size of the structure, and thus of the CPU*State > > structures. I don't think it's something we want to do, especially given > > we currently only use 7 flags, so 7 bits and that fits in a char. > > It does, but only by four bytes, which I didn't think was that > big a deal. If we want to keep it to one byte then I think Indeed it's not a lot, but if we do that with everything that goes into the CPU*state structures, it has a significant impact. > making it a uint8_t is probably better than leaving it as > signed char, given we're definitely not treating it as a > signed value. I agree the signed char type is very strange here, it is probably there for historical reasons. I guess using a uint8_t would indeed be the correct short term fix. The long term fix is probably to change this whole structure as a bitfield of unsigned int, like we already do in the tcg code. Aurelien
diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h index 43d0890..46b4091 100644 --- a/fpu/softfloat-specialize.h +++ b/fpu/softfloat-specialize.h @@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status) | should be simply `float_exception_flags |= flags;'. *----------------------------------------------------------------------------*/ -void float_raise(int8_t flags, float_status *status) +void float_raise(int flags, float_status *status) { status->float_exception_flags |= flags; } diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h index 0e57ee5..416cf7a 100644 --- a/include/fpu/softfloat.h +++ b/include/fpu/softfloat.h @@ -196,9 +196,9 @@ enum { }; typedef struct float_status { + int float_exception_flags; signed char float_detect_tininess; signed char float_rounding_mode; - signed char float_exception_flags; signed char floatx80_rounding_precision; /* should denormalised results go to zero and set the inexact flag? */ flag flush_to_zero; @@ -274,7 +274,7 @@ static inline flag get_default_nan_mode(float_status *status) | Routine to raise any or all of the software IEC/IEEE floating-point | exception flags. *----------------------------------------------------------------------------*/ -void float_raise(int8_t flags, float_status *status); +void float_raise(int flags, float_status *status); /*---------------------------------------------------------------------------- | If `a' is denormal and we are in flush-to-zero mode then set the
Change the flag type to 'int' to fix the implicit conversion error. Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> --- fpu/softfloat-specialize.h | 2 +- include/fpu/softfloat.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)