Re: [PATCH] Add new fp flags: -fassociative-math and -freciprocal-math

[Richard Guenther <rguenther at suse dot de>]

On Sun, 26 Aug 2007, Revital1 Eres wrote:

> Hello,
> 
> Following the BOF (http://gcc.gnu.org/wiki/FP_BOF) regarding
> floating points arithmetic in GCC that took place in the
> last summit, we updated the patch which introduces two
> new fp flags:  -fassociative-math and -freciprocal-math.
> (http://gcc.gnu.org/ml/gcc-patches/2006-11/msg01807.html)
> 
> Here is their description in a nutshell:
> 
> -fassociative-math - Allow optimization for floating-point arithmetic
> which allow re-association of operands.
> 
> -freciprocal-math - Allow the reciprocal of a value to be used instead
> of dividing by the value if this enables optimization.
> 
> Those two flags controls subsets of optimizations enabled
> by -funsafe-math-optimization.  The final goal is to replace
> -funsafe-math-optimizations with sets of flags; each one controls
> different subset of it.
> 
> This patch was bootstrapped and tested on ppc64 and x86_64.
> 
> OK for mainline?
>                                                                             
>  :ADDPATCH middle-end (fp arithmetic):                                      

The patch is ok with the following changes (find them inline in
the copy of the patch below).  Please wait a day or two for others
to comment though.

Thanks for your patience on this matter,
Richard.

:REVIEWMAIL:

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 127666)
+++ doc/invoke.texi	(working copy)
@@ -6170,6 +6170,8 @@
 
 @item -funsafe-math-optimizations
 @opindex funsafe-math-optimizations
+Sets @option{-freciprocal-math} and @option{-fassociative-math}.
+

Put this at the end of the funsafe-math-optinizations, use
"Enables" instead of "Sets".

@@ -6184,6 +6186,33 @@
 
 The default is @option{-fno-unsafe-math-optimizations}.
 
+@item -fassociative-math
+@opindex -fassociative-math
+
+-fassociative-math

Delete the two lines above.

+Allow re-association of operands in series of floating-point operations.
+This violates the ISO C and C++ language standard by possibly changing
+computation result.
+NOTE: re-ordering may change the sign of zero as well as ignore NaNs
+and inhibit or create underflow or overflow (and thus cannot be used
+on a code which relies on rounding behavior like (x + TWO52) - TWO52).
+May also reorder floating-point comparisons and thus may not be used
+when ordered comparisons are required.  This flag doesn't make much
+sense without -fno-signed-zeros, -fno-trapping-math or -frounding-math.

Wrap the options in the last sentence within @option{}.  Wrap the
example code (x + TWO52) - TWO52 in @code{}, I would also write
TWO52 as 2**52 for clarity.

+
+The default is @option{-fno-associative-math}.
+
+@item -freciprocal-math
+@opindex -freciprocal-math
+

No vertical spacing here.

+Allow the reciprocal of a value to be used instead of dividing by the
+value if this enables optimizations.  For example x / y can be replaced
+with x * (1/y) which is useful if (1/y) is subject to common subexpression
+elimination.  Note that this loses precision and increases the number
+of flops operating on the value.

Wrap the example code in @code{}.

+
+The default is @option{-fno-reciprocal-math}.
+
 @item -ffinite-math-only
 @opindex ffinite-math-only
 Allow optimizations for floating-point arithmetic that assume
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 127666)
+++ fold-const.c	(working copy)
@@ -9096,8 +9096,9 @@
 
       /* Likewise, we can simplify a comparison of a real constant with
          a MINUS_EXPR whose first operand is also a real constant, i.e.
-         (c1 - x) < c2 becomes x > c1-c2.  */
-      if (flag_unsafe_math_optimizations
+         (c1 - x) < c2 becomes x > c1-c2.  Reordering is allowed on 
+         floating-point types only if -fassociative-math is set.  */
+      if (flag_associative_math
 	  && TREE_CODE (arg1) == REAL_CST
 	  && TREE_CODE (arg0) == MINUS_EXPR
 	  && TREE_CODE (TREE_OPERAND (arg0, 0)) == REAL_CST
@@ -9655,7 +9656,7 @@
       if ((TREE_CODE (arg0) == MULT_EXPR
 	   || TREE_CODE (arg1) == MULT_EXPR)
 	  && !TYPE_SATURATING (type)
-	  && (!FLOAT_TYPE_P (type) || flag_unsafe_math_optimizations))
+	  && (!FLOAT_TYPE_P (type) || flag_associative_math))
         {
 	  tree tem = fold_plusminus_mult_expr (code, type, arg0, arg1);
 	  if (tem)

Please add to the comment before this hunk, fold_plusminus_mult_expr will
re-associate.

@@ -10209,7 +10214,7 @@
       if ((TREE_CODE (arg0) == MULT_EXPR
 	   || TREE_CODE (arg1) == MULT_EXPR)
 	  && !TYPE_SATURATING (type)
-	  && (!FLOAT_TYPE_P (type) || flag_unsafe_math_optimizations))
+	  && (!FLOAT_TYPE_P (type) || flag_associative_math))
         {
 	  tree tem = fold_plusminus_mult_expr (code, type, arg0, arg1);
 	  if (tem)

Likewise.

@@ -10300,8 +10305,10 @@
 	      && real_minus_onep (arg1))
 	    return fold_convert (type, negate_expr (arg0));
 
-	  /* Convert (C1/X)*C2 into (C1*C2)/X.  */
-	  if (flag_unsafe_math_optimizations
+	  /* Convert (C1/X)*C2 into (C1*C2)/X.  This transformation may change
+             the result for floating point types due to rounding so it is applied
+             only if -freciprocal-math was specify.  */
+	  if (flag_reciprocal_math
 	      && TREE_CODE (arg0) == RDIV_EXPR
 	      && TREE_CODE (arg1) == REAL_CST
 	      && TREE_CODE (TREE_OPERAND (arg0, 0)) == REAL_CST)

I believe this one should be testing flag_associative_math instead.  It is
merely re-associating the multiplication and the division like
(C1/X)*C2 -> C2*(C1/X) -> (C2*C1)/X.

@@ -10965,12 +10972,12 @@
 
       /* If ARG1 is a constant, we can convert this to a multiply by the
 	 reciprocal.  This does not have the same rounding properties,
-	 so only do this if -funsafe-math-optimizations.  We can actually
+	 so only do this if -freciprocal-math.  We can actually
 	 always safely do it if ARG1 is a power of two, but it's hard to
 	 tell if it is or not in a portable manner.  */
       if (TREE_CODE (arg1) == REAL_CST)
 	{
-	  if (flag_unsafe_math_optimizations
+	  if (flag_reciprocal_math
 	      && 0 != (tem = const_binop (code, build_real (type, dconst1),
 					  arg1, 0)))
 	    return fold_build2 (MULT_EXPR, type, arg0, tem);

We might relax this in future as the comment hints, but I guess flag_reciprocal_math
is as good as flag_unsafe_math_optimizations here.  Geert, if 1./CST is exactly
representable, how is rounding of x/CST different to X*(1./CST)?

@@ -10987,15 +10994,17 @@
 		}
 	    }
 	}
-      /* Convert A/B/C to A/(B*C).  */
-      if (flag_unsafe_math_optimizations
+      /* Convert A/B/C to A/(B*C).  This transformation may change
+         the result for floating point types due to rounding so it is applied
+         only if -freciprocal-math was specify.*/

The comment isn't exactly right - this isn't about rounding but chaning
the division and computation order here is going to hit values with greatly
differing magnitudes a lot (which is the whole business of separating out
flag_reciprocal_math).  I'd just leave the comment as it was before the patch.

+      if (flag_reciprocal_math
 	  && TREE_CODE (arg0) == RDIV_EXPR)
 	return fold_build2 (RDIV_EXPR, type, TREE_OPERAND (arg0, 0),
 			    fold_build2 (MULT_EXPR, type,
 					 TREE_OPERAND (arg0, 1), arg1));
Index: toplev.h
===================================================================
--- toplev.h	(revision 127666)
+++ toplev.h	(working copy)
@@ -146,6 +146,8 @@
 
 extern void set_fast_math_flags         (int);
 
+void set_unsafe_math_optimizations_flags (int);
+

Should be extern

Index: common.opt
===================================================================
--- common.opt	(revision 127666)
+++ common.opt	(working copy)
@@ -1125,6 +1125,29 @@
 Common Report Var(flag_unsafe_loop_optimizations) Optimization
 Allow loop optimizations to assume that the loops behave in normal way
 
+; Allow re-association of operands in series of floating-point operations.
+; This violates the ISO C and C++ language standard by possibly changing
+; computation result.
+; NOTE: re-ordering may change the sign of zero as well as ignore NaNs
+; and inhibit or create underflow or overflow (and thus cannot be used
+; on a code which relies on rounding behavior like (x + TWO52) - TWO52).
+; May also reorder floating-point comparisons and thus may not be used
+; when ordered comparisons are required.  This flag doesn't make much
+; sense without -fno-signed-zeros, -fno-trapping-math or -frounding-math.
+fassociative-math
+Common Report Var(flag_associative_math)
+Allow optimization for floating-point arithmetic which may change the
+result of the operation due to rounding.
+
+; Allow the reciprocal of a value to be used instead of dividing by the
+; value if this enables optimizations.  For example x / y can be replaced
+; with x * (1/y) which is useful if (1/y) is subject to common subexpression
+; elimination.  Note that this loses precision and increases the number
+; of flops operating on the value.
+freciprocal-math
+Common Report Var(flag_reciprocal_math)
+Same as -fassociative-math for expressions which include division.
+

Don't replicate the texi documentation here, no comment is ok.

Index: combine.c
===================================================================
--- combine.c	(revision 127666)
+++ combine.c	(working copy)
@@ -4687,7 +4687,7 @@
        || code == AND || code == IOR || code == XOR
        || code == SMAX || code == SMIN || code == UMAX || code == UMIN)
       && ((INTEGRAL_MODE_P (mode) && code != DIV)
-	  || (flag_unsafe_math_optimizations && FLOAT_MODE_P (mode))))
+	  || (flag_associative_math && FLOAT_MODE_P (mode))))
     {
       if (GET_CODE (XEXP (x, 0)) == code)
 	{
@@ -4960,7 +4960,7 @@
 	}
 
       /* Try simplify a*(b/c) as (a*b)/c.  */
-      if (FLOAT_MODE_P (mode) && flag_unsafe_math_optimizations
+      if (FLOAT_MODE_P (mode) && flag_reciprocal_math 
 	  && GET_CODE (XEXP (x, 0)) == DIV)
 	{
 	  rtx tem = simplify_binary_operation (MULT, mode,

This is, again, flag_associative_math.

@@ -8295,9 +8295,9 @@
   rtx tem;
 
   /* Distributivity is not true for floating point as it can change the
-     value.  So we don't do it unless -funsafe-math-optimizations.  */
+     value.  So we don't do it unless -fassociative-math.  */
   if (FLOAT_MODE_P (GET_MODE (x))
-      && ! flag_unsafe_math_optimizations)
+      && ! flag_associative_math)
     return x;

flag_associative_math is not sufficient here I think, so please leave
this hunk out (this one should be flag_contract_math or something like
it as it reduces the number of flops).