| Message ID | 1475254840-10972-1-git-send-email-james.greenhalgh@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 30 Sep 2016, James Greenhalgh wrote: > + case EXCESS_PRECISION_TYPE_STANDARD: > + case EXCESS_PRECISION_TYPE_IMPLICIT: > + /* Otherwise, the excess precision we want when we are > + in a standards compliant mode, and the implicit precision we > + provide can be identical. */ > + return FLT_EVAL_METHOD_PROMOTE_TO_DOUBLE; That's wrong for EXCESS_PRECISION_TYPE_IMPLICIT. There is no implicit promotion in the back end (and really there shouldn't be any excess precision here at all, and double_t in glibc should be fixed along with a GCC change to remove this mistake).
On 09/30/2016 11:34 AM, Joseph Myers wrote: > On Fri, 30 Sep 2016, James Greenhalgh wrote: > >> + case EXCESS_PRECISION_TYPE_STANDARD: >> + case EXCESS_PRECISION_TYPE_IMPLICIT: >> + /* Otherwise, the excess precision we want when we are >> + in a standards compliant mode, and the implicit precision we >> + provide can be identical. */ >> + return FLT_EVAL_METHOD_PROMOTE_TO_DOUBLE; > > That's wrong for EXCESS_PRECISION_TYPE_IMPLICIT. There is no implicit > promotion in the back end (and really there shouldn't be any excess > precision here at all, and double_t in glibc should be fixed along with a > GCC change to remove this mistake). Sorry, change to a NAK. Joseph, what's the right thing to do here? jeff
On Fri, 30 Sep 2016, Jeff Law wrote: > On 09/30/2016 11:34 AM, Joseph Myers wrote: > > On Fri, 30 Sep 2016, James Greenhalgh wrote: > > > > > + case EXCESS_PRECISION_TYPE_STANDARD: > > > + case EXCESS_PRECISION_TYPE_IMPLICIT: > > > + /* Otherwise, the excess precision we want when we are > > > + in a standards compliant mode, and the implicit precision we > > > + provide can be identical. */ > > > + return FLT_EVAL_METHOD_PROMOTE_TO_DOUBLE; > > > > That's wrong for EXCESS_PRECISION_TYPE_IMPLICIT. There is no implicit > > promotion in the back end (and really there shouldn't be any excess > > precision here at all, and double_t in glibc should be fixed along with a > > GCC change to remove this mistake). > Sorry, change to a NAK. > > Joseph, what's the right thing to do here? (a) The present patch would keep the existing value of FLT_EVAL_METHOD. But the existing value is inaccurate for the default compilation mode, when there is no implicit promotion in the back end, and doing so means suboptimal code in libgcc and glibc because it does things to handle excess precision that isn't actually there (and quite possibly in code elsewhere that looks at FLT_EVAL_METHOD). (b) Handling EXCESS_PRECISION_TYPE_IMPLICIT like EXCESS_PRECISION_TYPE_FAST would accurately describe what the back end does. It would mean that the default FLT_EVAL_METHOD is 0, which is a more accurate description of how the compiler actually behaves, and would avoid the suboptimal code in libgcc and glibc. It would however mean that unless -fexcess-precision=standard is used, FLT_EVAL_METHOD (accurate) is out of synx with float_t in math.h (inaccurate). (c) Removing all special excess precision for S/390 from GCC, and changing float_t to float in glibc, is logically correct and produces optimal code. float_t does not appear in the ABI of any glibc function; in principle it could affect the ABIs of other libraries, but I don't think that's particularly likely. The only argument for (a) is that's it's semantics-preserving - it's just that the preserved semantics are nonsensical and involve an inaccurate value of FLT_EVAL_METHOD in the default compilation mode.
On Fri, Sep 30, 2016 at 05:57:45PM +0000, Joseph Myers wrote: > On Fri, 30 Sep 2016, Jeff Law wrote: > > > On 09/30/2016 11:34 AM, Joseph Myers wrote: > > > On Fri, 30 Sep 2016, James Greenhalgh wrote: > > > > > > > + case EXCESS_PRECISION_TYPE_STANDARD: > > > > + case EXCESS_PRECISION_TYPE_IMPLICIT: > > > > + /* Otherwise, the excess precision we want when we are > > > > + in a standards compliant mode, and the implicit precision we > > > > + provide can be identical. */ > > > > + return FLT_EVAL_METHOD_PROMOTE_TO_DOUBLE; > > > > > > That's wrong for EXCESS_PRECISION_TYPE_IMPLICIT. There is no implicit > > > promotion in the back end (and really there shouldn't be any excess > > > precision here at all, and double_t in glibc should be fixed along with a > > > GCC change to remove this mistake). > > Sorry, change to a NAK. > > > > Joseph, what's the right thing to do here? > > (a) The present patch would keep the existing value of FLT_EVAL_METHOD. > But the existing value is inaccurate for the default compilation mode, > when there is no implicit promotion in the back end, and doing so means > suboptimal code in libgcc and glibc because it does things to handle > excess precision that isn't actually there (and quite possibly in code > elsewhere that looks at FLT_EVAL_METHOD). > > (b) Handling EXCESS_PRECISION_TYPE_IMPLICIT like > EXCESS_PRECISION_TYPE_FAST would accurately describe what the back end > does. It would mean that the default FLT_EVAL_METHOD is 0, which is a > more accurate description of how the compiler actually behaves, and would > avoid the suboptimal code in libgcc and glibc. It would however mean that > unless -fexcess-precision=standard is used, FLT_EVAL_METHOD (accurate) is > out of synx with float_t in math.h (inaccurate). > > (c) Removing all special excess precision for S/390 from GCC, and changing > float_t to float in glibc, is logically correct and produces optimal code. > float_t does not appear in the ABI of any glibc function; in principle it > could affect the ABIs of other libraries, but I don't think that's > particularly likely. > > The only argument for (a) is that's it's semantics-preserving - it's just > that the preserved semantics are nonsensical and involve an inaccurate > value of FLT_EVAL_METHOD in the default compilation mode. I'm happy progressing whichever of a) or b) would be preferred by the the s390 maintainers. But I'd be uncomfortable making the wider changes in c) as I've got no access to an s390 build and test environment in which I have any confidence, nor do I know the s390 port history that led to the 'typedef double float_t' in glibc. Regardless of which approach is chosen, I'll be sure to update the patch with a comment paraphrasing your suggestions above. Thanks, James
On 09/30/2016 07:57 PM, Joseph Myers wrote: > On Fri, 30 Sep 2016, Jeff Law wrote: > >> On 09/30/2016 11:34 AM, Joseph Myers wrote: >>> On Fri, 30 Sep 2016, James Greenhalgh wrote: >>> >>>> + case EXCESS_PRECISION_TYPE_STANDARD: >>>> + case EXCESS_PRECISION_TYPE_IMPLICIT: >>>> + /* Otherwise, the excess precision we want when we are >>>> + in a standards compliant mode, and the implicit precision we >>>> + provide can be identical. */ >>>> + return FLT_EVAL_METHOD_PROMOTE_TO_DOUBLE; >>> >>> That's wrong for EXCESS_PRECISION_TYPE_IMPLICIT. There is no implicit >>> promotion in the back end (and really there shouldn't be any excess >>> precision here at all, and double_t in glibc should be fixed along with a >>> GCC change to remove this mistake). >> Sorry, change to a NAK. >> >> Joseph, what's the right thing to do here? > > (a) The present patch would keep the existing value of FLT_EVAL_METHOD. > But the existing value is inaccurate for the default compilation mode, > when there is no implicit promotion in the back end, and doing so means > suboptimal code in libgcc and glibc because it does things to handle > excess precision that isn't actually there (and quite possibly in code > elsewhere that looks at FLT_EVAL_METHOD). > > (b) Handling EXCESS_PRECISION_TYPE_IMPLICIT like > EXCESS_PRECISION_TYPE_FAST would accurately describe what the back end > does. It would mean that the default FLT_EVAL_METHOD is 0, which is a > more accurate description of how the compiler actually behaves, and would > avoid the suboptimal code in libgcc and glibc. It would however mean that > unless -fexcess-precision=standard is used, FLT_EVAL_METHOD (accurate) is > out of synx with float_t in math.h (inaccurate). With (b) we would violate the C standard which explicitly states that the definition of float_t needs to be float if FLT_EVAL_METHOD is 0. I've no idea how much code really relies on that. So far I only know about the Plum Hall testsuite ;) So this probably would still be a safe change. Actually it was like that for many years without any problems ... until I've changed it due to the Plum Hall finding :( https://gcc.gnu.org/ml/gcc-patches/2013-03/msg01124.html > (c) Removing all special excess precision for S/390 from GCC, and changing > float_t to float in glibc, is logically correct and produces optimal code. > float_t does not appear in the ABI of any glibc function; in principle it > could affect the ABIs of other libraries, but I don't think that's > particularly likely. I really would like to do this. The idea came up several times already but we always were concerned about the potential ABI break. > The only argument for (a) is that's it's semantics-preserving - it's just > that the preserved semantics are nonsensical and involve an inaccurate > value of FLT_EVAL_METHOD in the default compilation mode. I'll try to set up some scans on src packages to get a better feel about where it would potentially break. I'll come back with the results. I do not want to block the patchset with this though. So if you would like to go on quickly feel free to commit (a). -Andreas-
On Tue, 4 Oct 2016, Andreas Krebbel wrote: > > (b) Handling EXCESS_PRECISION_TYPE_IMPLICIT like > > EXCESS_PRECISION_TYPE_FAST would accurately describe what the back end > > does. It would mean that the default FLT_EVAL_METHOD is 0, which is a > > more accurate description of how the compiler actually behaves, and would > > avoid the suboptimal code in libgcc and glibc. It would however mean that > > unless -fexcess-precision=standard is used, FLT_EVAL_METHOD (accurate) is > > out of synx with float_t in math.h (inaccurate). > > With (b) we would violate the C standard which explicitly states that > the definition of float_t needs to be float if FLT_EVAL_METHOD is 0. > I've no idea how much code really relies on that. So far I only know > about the Plum Hall testsuite ;) So this probably would still be a safe > change. Actually it was like that for many years without any problems > ... until I've changed it due to the Plum Hall finding :( > https://gcc.gnu.org/ml/gcc-patches/2013-03/msg01124.html You'd only violate it outside standards conformance modes (which you should be using for running conformance testsuites); with -std=c11 etc. -fexcess-precision=standard would be implied, meaning FLT_EVAL_METHOD remains as 1 in that case.
On 10/04/2016 03:42 PM, Joseph Myers wrote: > On Tue, 4 Oct 2016, Andreas Krebbel wrote: > >>> (b) Handling EXCESS_PRECISION_TYPE_IMPLICIT like >>> EXCESS_PRECISION_TYPE_FAST would accurately describe what the back end >>> does. It would mean that the default FLT_EVAL_METHOD is 0, which is a >>> more accurate description of how the compiler actually behaves, and would >>> avoid the suboptimal code in libgcc and glibc. It would however mean that >>> unless -fexcess-precision=standard is used, FLT_EVAL_METHOD (accurate) is >>> out of synx with float_t in math.h (inaccurate). >> >> With (b) we would violate the C standard which explicitly states that >> the definition of float_t needs to be float if FLT_EVAL_METHOD is 0. >> I've no idea how much code really relies on that. So far I only know >> about the Plum Hall testsuite ;) So this probably would still be a safe >> change. Actually it was like that for many years without any problems >> ... until I've changed it due to the Plum Hall finding :( >> https://gcc.gnu.org/ml/gcc-patches/2013-03/msg01124.html > > You'd only violate it outside standards conformance modes (which you > should be using for running conformance testsuites); with -std=c11 etc. > -fexcess-precision=standard would be implied, meaning FLT_EVAL_METHOD > remains as 1 in that case. wrt (b): Agreed. I was more concerned about all the other code which might accidently be built without a strict standard compliance option. I did some searches in the source package repos. The only snippet where the definition of FLT_EVAL_METHOD might affect code generation is in musl libc but that one is being built with -std=c99. So I don't see anything speaking against (b). I'm ok with going that way. wrt (c): float_t appears to be more widely used than I expected. But the only hits which might indicate potential ABI problems where in clucene and libassa. (I've scanned the header files of about 25k Ubuntu source packages). I'm also not sure about script language interfaces with C. There might be potential problems out there which I wasn't able to catch with my scan. While I fully agree with you that the float_t type definition for S/390 in Glibc is plain wrong I do not really feel comfortable with changing it. An interesting case is imagemagick. They define their ABI-relevant MagickRealType based on the size of float_t in recent versions but excplicitly without depending on FLT_EVAL_METHOD (http://www.imagemagick.org/discourse-server/viewtopic.php?t=22136). They build with -std=gnu99 so this helps us with (b) I think. To my understanding MagickRealType would stay double not affected by FLT_EVAL_METHOD changes. -Andreas-
On Fri, 7 Oct 2016, Andreas Krebbel wrote: > wrt (c): float_t appears to be more widely used than I expected. But the > only hits which might indicate potential ABI problems where in clucene > and libassa. (I've scanned the header files of about 25k Ubuntu source > packages). If it's two out of 25000 source packages whose ABIs might be affected, I think that shows it's much safer as a change in glibc than moving to _FILE_OFFSET_BITS=64 as a default (which I expect will happen when someone puts the work in). And probably safer than many past changes done through symbol versioning.
On 10/07/2016 03:11 PM, Joseph Myers wrote: > On Fri, 7 Oct 2016, Andreas Krebbel wrote: > >> wrt (c): float_t appears to be more widely used than I expected. But the >> only hits which might indicate potential ABI problems where in clucene >> and libassa. (I've scanned the header files of about 25k Ubuntu source >> packages). > > If it's two out of 25000 source packages whose ABIs might be affected, I > think that shows it's much safer as a change in glibc than moving to > _FILE_OFFSET_BITS=64 as a default (which I expect will happen when someone > puts the work in). And probably safer than many past changes done through > symbol versioning. Regarding (c) imagemagick is also affected (it wasn't really clear from my last email). Since it is a widely used lib I think this counts as a blocker. The ABI relevant MagickRealType depends on the size of float_t: /* Float_t is not an ABI type. */ #if MAGICKCORE_SIZEOF_FLOAT_T == 0 typedef float MagickRealType; #elif (MAGICKCORE_SIZEOF_FLOAT_T == MAGICKCORE_SIZEOF_FLOAT) typedef float MagickRealType; #elif (MAGICKCORE_SIZEOF_FLOAT_T == MAGICKCORE_SIZEOF_DOUBLE) typedef double MagickRealType; #elif (MAGICKCORE_SIZEOF_FLOAT_T == MAGICKCORE_SIZEOF_LONG_DOUBLE) typedef long double MagickRealType; #else # error Your float_t type is neither a float, nor a double, nor a long double #endif So I would prefer (b) which looks like a good compromise to me. -Andreas-
On Wed, 12 Oct 2016, Andreas Krebbel wrote: > Regarding (c) imagemagick is also affected (it wasn't really clear from > my last email). Since it is a widely used lib I think this counts as a > blocker. The ABI relevant MagickRealType depends on the size of float_t: I think distributions manage ABI transitions for such libraries all the time (and could well choose to patch it locally to defer the change until it next changes SONAME upstream anyway).
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c index 3bdb648..b704d46 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -15106,6 +15106,34 @@ s390_invalid_binary_op (int op ATTRIBUTE_UNUSED, const_tree type1, const_tree ty return NULL; } +/* Implement TARGET_EXCESS_PRECISION. + + This is used by float.h to define the float_t and double_t data + types. For historical reasons both are double on s390 what cannot + be changed anymore. */ + +static enum flt_eval_method +s390_excess_precision (enum excess_precision_type type) +{ + switch (type) + { + case EXCESS_PRECISION_TYPE_FAST: + /* The fastest type to promote to will always be the native type, + whether that occurs with implicit excess precision or + otherwise. */ + return FLT_EVAL_METHOD_PROMOTE_TO_FLOAT; + case EXCESS_PRECISION_TYPE_STANDARD: + case EXCESS_PRECISION_TYPE_IMPLICIT: + /* Otherwise, the excess precision we want when we are + in a standards compliant mode, and the implicit precision we + provide can be identical. */ + return FLT_EVAL_METHOD_PROMOTE_TO_DOUBLE; + default: + gcc_unreachable (); + } + return FLT_EVAL_METHOD_UNPREDICTABLE; +} + /* Initialize GCC target structure. */ #undef TARGET_ASM_ALIGNED_HI_OP @@ -15161,6 +15189,9 @@ s390_invalid_binary_op (int op ATTRIBUTE_UNUSED, const_tree type1, const_tree ty #undef TARGET_ASM_CAN_OUTPUT_MI_THUNK #define TARGET_ASM_CAN_OUTPUT_MI_THUNK hook_bool_const_tree_hwi_hwi_const_tree_true +#undef TARGET_C_EXCESS_PRECISION +#define TARGET_C_EXCESS_PRECISION s390_excess_precision + #undef TARGET_SCHED_ADJUST_PRIORITY #define TARGET_SCHED_ADJUST_PRIORITY s390_adjust_priority #undef TARGET_SCHED_ISSUE_RATE