[PATCH][C-Family] Disable bogus shortening of DFP vs FP compare

[PATCH][C-Family] Disable bogus shortening of DFP vs FP compare

[Richard Biener]

On the match-and-simplify branch we expose an issue in shorten_compare
which happily transforms (double) float-var != (double) dfp-float-var
to (float) float-var != (float) dfp-float-var which is wrong
and causes

FAIL: c-c++-common/dfp/convert-bfp-12.c  -std=c++11 execution test
FAIL: c-c++-common/dfp/convert-bfp-12.c  -std=c++1y execution test
FAIL: c-c++-common/dfp/convert-bfp-12.c  -std=c++98 execution test

you can expose this latent bug by making get_narrower also
treat CONVERT_EXPRs as conversions - on trunk the DFP value
happens to be converted to double using that.

You then also run into the issue that the C frontend rejects
the compare via its common_type implementation which complains
about a mixed FP - DFP compare.

So the following patch which I am testing right now disables
the (premature) shorten-compare optimization on mixed
FP - DFP operands.  It also exposes the issue on trunk by
improving get_narrower.

Ok for trunk either with or without the tree.c hunk?  (I'm
going to remove that if it exposes more issues elsewhere
revealed by testing)

Thanks,
Richard.

2014-11-04  Richard Biener  <rguenther@suse.de>

	* tree.c (get_narrower): Also look through CONVERT_EXPRs.

	c-family/
	* c-common.c (shorten_compare): Do not shorten mixed
	DFP and non-DFP compares.

Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 217049)
+++ gcc/tree.c	(working copy)
@@ -8494,7 +8494,7 @@ get_narrower (tree op, int *unsignedp_pt
   tree win = op;
   bool integral_p = INTEGRAL_TYPE_P (TREE_TYPE (op));
 
-  while (TREE_CODE (op) == NOP_EXPR)
+  while (CONVERT_EXPR_P (op))
     {
       int bitschange
 	= (TYPE_PRECISION (TREE_TYPE (op))
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 217049)
+++ gcc/c-family/c-common.c	(working copy)
@@ -4314,9 +4314,15 @@ shorten_compare (location_t loc, tree *o
   /* If either arg is decimal float and the other is float, find the
      proper common type to use for comparison.  */
   else if (real1 && real2
+	   && DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop0)))
+	   && DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop1))))
+    type = common_type (TREE_TYPE (primop0), TREE_TYPE (primop1));
+
+  /* If either arg is decimal float and the other is float, fail.  */
+  else if (real1 && real2
 	   && (DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop0)))
 	       || DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop1)))))
-    type = common_type (TREE_TYPE (primop0), TREE_TYPE (primop1));
+    return 0;
 
   else if (real1 && real2
 	   && (TYPE_PRECISION (TREE_TYPE (primop0))

Re: [PATCH][C-Family] Disable bogus shortening of DFP vs FP compare

[Richard Biener]

On Tue, 4 Nov 2014, Richard Biener wrote:

> 
> On the match-and-simplify branch we expose an issue in shorten_compare
> which happily transforms (double) float-var != (double) dfp-float-var
> to (float) float-var != (float) dfp-float-var which is wrong
> and causes
> 
> FAIL: c-c++-common/dfp/convert-bfp-12.c  -std=c++11 execution test
> FAIL: c-c++-common/dfp/convert-bfp-12.c  -std=c++1y execution test
> FAIL: c-c++-common/dfp/convert-bfp-12.c  -std=c++98 execution test
> 
> you can expose this latent bug by making get_narrower also
> treat CONVERT_EXPRs as conversions - on trunk the DFP value
> happens to be converted to double using that.
> 
> You then also run into the issue that the C frontend rejects
> the compare via its common_type implementation which complains
> about a mixed FP - DFP compare.
> 
> So the following patch which I am testing right now disables
> the (premature) shorten-compare optimization on mixed
> FP - DFP operands.  It also exposes the issue on trunk by
> improving get_narrower.
> 
> Ok for trunk either with or without the tree.c hunk?  (I'm
> going to remove that if it exposes more issues elsewhere
> revealed by testing)

I removed it - it fails very early in stage2 with a warning about an
always-true comparison.  No time to sort that out at this point.
So consider this a c-common.c change only.

Thanks,
Richard.

> 2014-11-04  Richard Biener  <rguenther@suse.de>
> 
> 	* tree.c (get_narrower): Also look through CONVERT_EXPRs.
> 
> 	c-family/
> 	* c-common.c (shorten_compare): Do not shorten mixed
> 	DFP and non-DFP compares.
> 
> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c	(revision 217049)
> +++ gcc/tree.c	(working copy)
> @@ -8494,7 +8494,7 @@ get_narrower (tree op, int *unsignedp_pt
>    tree win = op;
>    bool integral_p = INTEGRAL_TYPE_P (TREE_TYPE (op));
>  
> -  while (TREE_CODE (op) == NOP_EXPR)
> +  while (CONVERT_EXPR_P (op))
>      {
>        int bitschange
>  	= (TYPE_PRECISION (TREE_TYPE (op))
> Index: gcc/c-family/c-common.c
> ===================================================================
> --- gcc/c-family/c-common.c	(revision 217049)
> +++ gcc/c-family/c-common.c	(working copy)
> @@ -4314,9 +4314,15 @@ shorten_compare (location_t loc, tree *o
>    /* If either arg is decimal float and the other is float, find the
>       proper common type to use for comparison.  */
>    else if (real1 && real2
> +	   && DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop0)))
> +	   && DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop1))))
> +    type = common_type (TREE_TYPE (primop0), TREE_TYPE (primop1));
> +
> +  /* If either arg is decimal float and the other is float, fail.  */
> +  else if (real1 && real2
>  	   && (DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop0)))
>  	       || DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (primop1)))))
> -    type = common_type (TREE_TYPE (primop0), TREE_TYPE (primop1));
> +    return 0;
>  
>    else if (real1 && real2
>  	   && (TYPE_PRECISION (TREE_TYPE (primop0))
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendoerffer, HRB 21284
(AG Nuernberg)
Maxfeldstrasse 5, 90409 Nuernberg, Germany

Re: [PATCH][C-Family] Disable bogus shortening of DFP vs FP compare

[Joseph Myers]

On Tue, 4 Nov 2014, Richard Biener wrote:

> 	c-family/
> 	* c-common.c (shorten_compare): Do not shorten mixed
> 	DFP and non-DFP compares.

OK.  I think it's also wrong for get_narrower to strip conversions between 
binary and decimal floating point at all, as all such conversions for 
supported pairs of types can change values.  (_Decimal128 can represent 
all values of __fp16, but no target currently supports both types.)  And 
if flag_signaling_nans (strictly, if HONOR_SNANS (source-mode)), no 
floating-point conversions should be stripped in get_narrower at all.  And 
even without signaling NaNs, TYPE_PRECISION may not be a reliable 
indication of whether a conversion is widening - the only issue applying 
at present might be the corner case that a conversion of a subnormal from 
__float80 to __float128 is exact but still raises "underflow" if traps on 
underflow are enabled, but when you have both binary128 and IBM long 
double supported together then neither has a set of values that is a 
superset of the other (it seems 
<https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01086.html> misses 
get_narrower in the places that need addressing).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH][C-Family] Disable bogus shortening of DFP vs FP compare

[Richard Biener]

On Tue, 4 Nov 2014, Joseph Myers wrote:

> On Tue, 4 Nov 2014, Richard Biener wrote:
> 
> > 	c-family/
> > 	* c-common.c (shorten_compare): Do not shorten mixed
> > 	DFP and non-DFP compares.
> 
> OK.  I think it's also wrong for get_narrower to strip conversions between 
> binary and decimal floating point at all, as all such conversions for 
> supported pairs of types can change values.  (_Decimal128 can represent 
> all values of __fp16, but no target currently supports both types.)  And 
> if flag_signaling_nans (strictly, if HONOR_SNANS (source-mode)), no 
> floating-point conversions should be stripped in get_narrower at all.  And 
> even without signaling NaNs, TYPE_PRECISION may not be a reliable 
> indication of whether a conversion is widening - the only issue applying 
> at present might be the corner case that a conversion of a subnormal from 
> __float80 to __float128 is exact but still raises "underflow" if traps on 
> underflow are enabled, but when you have both binary128 and IBM long 
> double supported together then neither has a set of values that is a 
> superset of the other (it seems 
> <https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01086.html> misses 
> get_narrower in the places that need addressing).

From this and past issues it looks like most (if not all) conversions
between FP types use CONVERT_EXPR, not NOP_EXPR and thus may work
"as expected" here.  That's of course somewhat unreliable as the
middle-end happily uses NOP_EXPR for those.

Richard.