Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Andreas Krebbel]

On 10/17/2016 09:29 PM, Andreas Krebbel1 wrote:
>> Here is a patch implementing what I think has been discussed in this 
> thread.
>>
>> OK?
> 
> Looks good to me.
> 
> Uli, do you agree with that change for S/390 or would you rather see us 
> fixing the float_t type definition in Glibc?

I had a discussion with Ulrich. He agrees with the current patch. So the patch 
is ok to apply.
Thanks for taking care of this (and being patient with me).

Also I will try to push forward changing float_t to float. Your patch does not 
make things worse and
should not make it harder to do the float_t switch in the future. For the 
float_t switch I will have
to check with the distro maintainers.

-Andreas-

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Andreas Krebbel1]

> Here is a patch implementing what I think has been discussed in this 
thread.
> 
> OK?

Looks good to me.

Uli, do you agree with that change for S/390 or would you rather see us 
fixing the float_t type definition in Glibc?

-Andreas-

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[James Greenhalgh]

Hi,

On Fri, Oct 07, 2016 at 10:34:25AM +0200, Andreas Krebbel wrote:
> 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.

Here is a patch implementing what I think has been discussed in this thread.

OK?

Thanks,
James

---
gcc/

2016-10-14  James Greenhalgh  

* config/s390/s390.c (s390_excess_precision): New.
(TARGET_C_EXCESS_PRECISION): Define.

diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index f69b470..8f6f199 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -15107,6 +15107,43 @@ s390_invalid_binary_op (int op ATTRIBUTE_UNUSED, const_tree type1, const_tree ty
   return NULL;
 }
 
+/* Implement TARGET_C_EXCESS_PRECISION.
+
+   FIXME: For historical reasons, float_t and double_t are typedef'ed to
+   double on s390, causing operations on float_t to operate in a higher
+   precision than is necessary.  However, it is not the case that SFmode
+   operations have implicit excess precision, and we generate more optimal
+   code if we let the compiler know no implicit extra precision is added.
+
+   That means when we are compiling with -fexcess-precision=fast, the value
+   we set for FLT_EVAL_METHOD will be out of line with the actual precision of
+   float_t (though they would be correct for -fexcess-precision=standard).
+
+   A complete fix would modify glibc to remove the unnecessary typedef
+   of float_t to double.  */
+
+static enum flt_eval_method
+s390_excess_precision (enum excess_precision_type type)
+{
+  switch (type)
+{
+  case EXCESS_PRECISION_TYPE_IMPLICIT:
+  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:
+	/* Otherwise, when we are in a standards compliant mode, to
+	   ensure consistency with the implementation in glibc, report that
+	   float is evaluated to the range and precision of double.  */
+	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
@@ 

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Joseph Myers]

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).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Andreas Krebbel]

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-

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Joseph Myers]

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.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Andreas Krebbel]

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-

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Joseph Myers]

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.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Andreas Krebbel]

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-

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[James Greenhalgh]

On Fri, Sep 30, 2016 at 05:57:45PM +, 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

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Joseph Myers]

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.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Jeff Law]

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

Re: [Patch 3/11] Implement TARGET_C_EXCESS_PRECISION for s390

[Joseph Myers]

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).

-- 
Joseph S. Myers
jos...@codesourcery.com