| Message ID | 20180109122252.17670-3-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | re-factor softfloat and add fp16 functions | expand |
Le 09/01/2018 à 13:22, Alex Bennée a écrit : > It's not actively built and when enabled things fail to compile. I'm > not sure the type-checking is really helping here. Seeing as we "own" > our softfloat now lets remove the cruft. I think it would be better to fix the build break than to remove the type-checking tool. but that's only my opinion... Thanks, Laurent
On 2018-01-09 13:27, Laurent Vivier wrote: > Le 09/01/2018 à 13:22, Alex Bennée a écrit : > > It's not actively built and when enabled things fail to compile. I'm > > not sure the type-checking is really helping here. Seeing as we "own" > > our softfloat now lets remove the cruft. > > I think it would be better to fix the build break than to remove the > type-checking tool. > > but that's only my opinion... I agree with that. Those checks are useful for targets which call host floating point functions for some instructions. This is ugly, but that's what is still done for at least x86 for the trigonometrical functions. The check prevents assigning a float or double value to a softfloat type without calling the conversion function. Now, when we make sure that those ugly things are removed, I think these type-checking might be removed. Aurelien -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net
On 9 January 2018 at 14:12, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2018-01-09 13:27, Laurent Vivier wrote: >> Le 09/01/2018 à 13:22, Alex Bennée a écrit : >> > It's not actively built and when enabled things fail to compile. I'm >> > not sure the type-checking is really helping here. Seeing as we "own" >> > our softfloat now lets remove the cruft. >> >> I think it would be better to fix the build break than to remove the >> type-checking tool. >> >> but that's only my opinion... > > I agree with that. Those checks are useful for targets which call host > floating point functions for some instructions. This is ugly, but that's > what is still done for at least x86 for the trigonometrical functions. > The check prevents assigning a float or double value to a softfloat type > without calling the conversion function. Is gcc's codegen still bad enough that we have to default to not using the type-checking versions? If so, maybe we could at least enable the type-checking on an --enable-debug build, so it doesn't bitrot all the time. thanks -- PMM
Le 09/01/2018 à 15:14, Peter Maydell a écrit : > On 9 January 2018 at 14:12, Aurelien Jarno <aurelien@aurel32.net> wrote: >> On 2018-01-09 13:27, Laurent Vivier wrote: >>> Le 09/01/2018 à 13:22, Alex Bennée a écrit : >>>> It's not actively built and when enabled things fail to compile. I'm >>>> not sure the type-checking is really helping here. Seeing as we "own" >>>> our softfloat now lets remove the cruft. >>> >>> I think it would be better to fix the build break than to remove the >>> type-checking tool. >>> >>> but that's only my opinion... >> >> I agree with that. Those checks are useful for targets which call host >> floating point functions for some instructions. This is ugly, but that's >> what is still done for at least x86 for the trigonometrical functions. >> The check prevents assigning a float or double value to a softfloat type >> without calling the conversion function. > > Is gcc's codegen still bad enough that we have to default to not > using the type-checking versions? If so, maybe we could at least > enable the type-checking on an --enable-debug build, so it doesn't > bitrot all the time. What means "bad enough"? for some targets it works fine. The problem with that is if it is not enabled all the time it becomes broken really quick... BTW, if it doesn't break Alex's work I'm volunteer to fix USE_SOFTFLOAT_STRUCT_TYPES build. Thanks, Laurent
On 9 January 2018 at 14:20, Laurent Vivier <laurent@vivier.eu> wrote: > Le 09/01/2018 à 15:14, Peter Maydell a écrit : >> Is gcc's codegen still bad enough that we have to default to not >> using the type-checking versions? If so, maybe we could at least >> enable the type-checking on an --enable-debug build, so it doesn't >> bitrot all the time. > > What means "bad enough"? for some targets it works fine. I mean whatever the problem was that made us write the comment A sufficiently clever compiler and sane ABI should be able to see though these structs. However x86/gcc 3.x seems to struggle a bit, so leave them disabled by default. In theory the code generated should be no worse than without structs... > The problem with that is if it is not enabled all the time it becomes > broken really quick... Yes, that's why I'd like it at least default-enabled for --enable-debug, if we can't enable it always. thanks -- PMM
Laurent Vivier <laurent@vivier.eu> writes: > Le 09/01/2018 à 15:14, Peter Maydell a écrit: >> On 9 January 2018 at 14:12, Aurelien Jarno <aurelien@aurel32.net> wrote: >>> On 2018-01-09 13:27, Laurent Vivier wrote: >>>> Le 09/01/2018 à 13:22, Alex Bennée a écrit : >>>>> It's not actively built and when enabled things fail to compile. I'm >>>>> not sure the type-checking is really helping here. Seeing as we "own" >>>>> our softfloat now lets remove the cruft. >>>> >>>> I think it would be better to fix the build break than to remove the >>>> type-checking tool. >>>> >>>> but that's only my opinion... >>> >>> I agree with that. Those checks are useful for targets which call host >>> floating point functions for some instructions. This is ugly, but that's >>> what is still done for at least x86 for the trigonometrical functions. >>> The check prevents assigning a float or double value to a softfloat type >>> without calling the conversion function. >> >> Is gcc's codegen still bad enough that we have to default to not >> using the type-checking versions? If so, maybe we could at least >> enable the type-checking on an --enable-debug build, so it doesn't >> bitrot all the time. > > What means "bad enough"? for some targets it works fine. > > The problem with that is if it is not enabled all the time it becomes > broken really quick... > > BTW, if it doesn't break Alex's work I'm volunteer to fix > USE_SOFTFLOAT_STRUCT_TYPES build. Be my guest - I suspect getting that merged would be on a faster path than the rest of the softfloat re-factoring patch series (unless the relative silence means everyone is happy with it ;-). By the way I mentioned in my header mail that the types are included from bswap.h so it would be nice to move the softfloat types somewhere where they could be more easily included to avoid triggering a re-build every time softfloat.h is touched. -- Alex Bennée
On 01/09/2018 06:43 AM, Peter Maydell wrote: > On 9 January 2018 at 14:20, Laurent Vivier <laurent@vivier.eu> wrote: >> Le 09/01/2018 à 15:14, Peter Maydell a écrit : >>> Is gcc's codegen still bad enough that we have to default to not >>> using the type-checking versions? If so, maybe we could at least >>> enable the type-checking on an --enable-debug build, so it doesn't >>> bitrot all the time. >> >> What means "bad enough"? for some targets it works fine. > > I mean whatever the problem was that made us write the comment > A sufficiently clever compiler and > sane ABI should be able to see though these structs. However > x86/gcc 3.x seems to struggle a bit, so leave them disabled by default. E.g. the i386 ABI is *not* sane in this respect. Nor is sparcv8plus. I'd have to check on the others. If we enable USE_SOFTFLOAT_STRUCT_TYPES, then we *must* remove the markup for f32 and f64 from include/exec/helper-head.h, because structures are passed differently from integers as parameters and/or return values. I personally do not think USE_SOFTFLOAT_STRUCT_TYPES is worth the headache. r~
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h index d5e99667b6..52af1412de 100644 --- a/include/fpu/softfloat.h +++ b/include/fpu/softfloat.h @@ -103,32 +103,6 @@ enum { /*---------------------------------------------------------------------------- | Software IEC/IEEE floating-point types. *----------------------------------------------------------------------------*/ -/* Use structures for soft-float types. This prevents accidentally mixing - them with native int/float types. A sufficiently clever compiler and - sane ABI should be able to see though these structs. However - x86/gcc 3.x seems to struggle a bit, so leave them disabled by default. */ -//#define USE_SOFTFLOAT_STRUCT_TYPES -#ifdef USE_SOFTFLOAT_STRUCT_TYPES -typedef struct { - uint16_t v; -} float16; -#define float16_val(x) (((float16)(x)).v) -#define make_float16(x) __extension__ ({ float16 f16_val = {x}; f16_val; }) -#define const_float16(x) { x } -typedef struct { - uint32_t v; -} float32; -/* The cast ensures an error if the wrong type is passed. */ -#define float32_val(x) (((float32)(x)).v) -#define make_float32(x) __extension__ ({ float32 f32_val = {x}; f32_val; }) -#define const_float32(x) { x } -typedef struct { - uint64_t v; -} float64; -#define float64_val(x) (((float64)(x)).v) -#define make_float64(x) __extension__ ({ float64 f64_val = {x}; f64_val; }) -#define const_float64(x) { x } -#else typedef uint16_t float16; typedef uint32_t float32; typedef uint64_t float64; @@ -141,7 +115,6 @@ typedef uint64_t float64; #define const_float16(x) (x) #define const_float32(x) (x) #define const_float64(x) (x) -#endif typedef struct { uint64_t low; uint16_t high;
It's not actively built and when enabled things fail to compile. I'm not sure the type-checking is really helping here. Seeing as we "own" our softfloat now lets remove the cruft. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/fpu/softfloat.h | 27 --------------------------- 1 file changed, 27 deletions(-) -- 2.15.1