diff mbox

[3/11] Implement TARGET_C_EXCESS_PRECISION for s390

Message ID 1476463964-23638-1-git-send-email-james.greenhalgh@arm.com
State New
Headers show

Commit Message

James Greenhalgh Oct. 14, 2016, 4:52 p.m. UTC 
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  <james.greenhalgh@arm.com>

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

Comments

Andreas Krebbel1 Oct. 17, 2016, 7:29 p.m. UTC | #1 
> 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-
Andreas Krebbel Oct. 19, 2016, 3:20 p.m. UTC | #2 
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-
diff mbox

Patch

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
@@ -15167,6 +15204,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