Fix PR tree-optimization/80426

Submitted by Eric Botcazou on April 19, 2017, 2:59 p.m.

Details

Message ID 12981441.W0lox0l1Ju@polaris
State New
Headers show

Commit Message

Eric Botcazou April 19, 2017, 2:59 p.m.
> I know this attempts to be a copy of what is used elsewhere, but
> at least there it is a result of wi::sub etc.
> Wouldn't it be simpler to
>   if (sgn == SIGNED && wi::neg_p (min_op1) && wi::neg_p (wmin))
>     min_ovf = 1;
>   else if (sgn == UNSIGNED && wi::ne_p (min_op1, 0))
>     min_ovf = -1;
> 
> I mean, for SIGNED if min_op1 is 0, then wmin is 0 to and we want
> min_ovf = 0;
> If it is positive, wmin will be surely negative and again we want
> min_ovf = 0.  Only if it is negative and its negation is negative
> too we want min_ovf = 1 (i.e. wi::cmps (0, most_negative) result).
> For UNSIGNED, if min_op1 is 0, again all 3 wi::cmp will yield
> 0 and min_ovf = 0.  If it is non-zero, it is > 0, therefore it
> the first wi::cmp will return -1, the second wi::cmp returns
> 1 and the third one -1.

Fine with me.

> Is that what we want (e.g. the UNSIGNED case to overflow pretty much always
> except for 0 which should be optimized away anyway)?

I think so, you'd better be very cautious with overflow and symbolic ranges.

> Or, shouldn't we just set if (!min_op0 && min_op1 && minus_p) min_op0 =
> build_int_cst (expr_type, 0); before the if (min_op0 && min_op1) case
> and don't duplicate that?

This isn't better than my version IMO.

Tested on x86_64-suse-linux, OK for mainline and 6 branch?


2017-04-19  Eric Botcazou  <ebotcazou@adacore.com>
            Jakub Jelinek  <jakub@redhat.com>

        PR tree-optimization/80426
        * tree-vrp.c (extract_range_from_binary_expr_1): For an additive
        operation on symbolic operands, also compute the overflow for the
        invariant part when the operation degenerates into a negation.

Comments

Jakub Jelinek April 19, 2017, 3:05 p.m.
On Wed, Apr 19, 2017 at 04:59:50PM +0200, Eric Botcazou wrote:
> > I know this attempts to be a copy of what is used elsewhere, but
> > at least there it is a result of wi::sub etc.
> > Wouldn't it be simpler to
> >   if (sgn == SIGNED && wi::neg_p (min_op1) && wi::neg_p (wmin))
> >     min_ovf = 1;
> >   else if (sgn == UNSIGNED && wi::ne_p (min_op1, 0))
> >     min_ovf = -1;
> > 
> > I mean, for SIGNED if min_op1 is 0, then wmin is 0 to and we want
> > min_ovf = 0;
> > If it is positive, wmin will be surely negative and again we want
> > min_ovf = 0.  Only if it is negative and its negation is negative
> > too we want min_ovf = 1 (i.e. wi::cmps (0, most_negative) result).
> > For UNSIGNED, if min_op1 is 0, again all 3 wi::cmp will yield
> > 0 and min_ovf = 0.  If it is non-zero, it is > 0, therefore it
> > the first wi::cmp will return -1, the second wi::cmp returns
> > 1 and the third one -1.
> 
> Fine with me.
> 
> > Is that what we want (e.g. the UNSIGNED case to overflow pretty much always
> > except for 0 which should be optimized away anyway)?
> 
> I think so, you'd better be very cautious with overflow and symbolic ranges.
> 
> > Or, shouldn't we just set if (!min_op0 && min_op1 && minus_p) min_op0 =
> > build_int_cst (expr_type, 0); before the if (min_op0 && min_op1) case
> > and don't duplicate that?
> 
> This isn't better than my version IMO.
> 
> Tested on x86_64-suse-linux, OK for mainline and 6 branch?

Ok, thanks.

	Jakub
Jeff Law April 19, 2017, 7:26 p.m.
On 04/19/2017 08:59 AM, Eric Botcazou wrote:
>> I know this attempts to be a copy of what is used elsewhere, but
>> at least there it is a result of wi::sub etc.
>> Wouldn't it be simpler to
>>    if (sgn == SIGNED && wi::neg_p (min_op1) && wi::neg_p (wmin))
>>      min_ovf = 1;
>>    else if (sgn == UNSIGNED && wi::ne_p (min_op1, 0))
>>      min_ovf = -1;
>>
>> I mean, for SIGNED if min_op1 is 0, then wmin is 0 to and we want
>> min_ovf = 0;
>> If it is positive, wmin will be surely negative and again we want
>> min_ovf = 0.  Only if it is negative and its negation is negative
>> too we want min_ovf = 1 (i.e. wi::cmps (0, most_negative) result).
>> For UNSIGNED, if min_op1 is 0, again all 3 wi::cmp will yield
>> 0 and min_ovf = 0.  If it is non-zero, it is > 0, therefore it
>> the first wi::cmp will return -1, the second wi::cmp returns
>> 1 and the third one -1.
> 
> Fine with me.
> 
>> Is that what we want (e.g. the UNSIGNED case to overflow pretty much always
>> except for 0 which should be optimized away anyway)?
> 
> I think so, you'd better be very cautious with overflow and symbolic ranges.
> 
>> Or, shouldn't we just set if (!min_op0 && min_op1 && minus_p) min_op0 =
>> build_int_cst (expr_type, 0); before the if (min_op0 && min_op1) case
>> and don't duplicate that?
> 
> This isn't better than my version IMO.
> 
> Tested on x86_64-suse-linux, OK for mainline and 6 branch?
> 
> 
> 2017-04-19  Eric Botcazou  <ebotcazou@adacore.com>
>              Jakub Jelinek  <jakub@redhat.com>
> 
>          PR tree-optimization/80426
>          * tree-vrp.c (extract_range_from_binary_expr_1): For an additive
>          operation on symbolic operands, also compute the overflow for the
>          invariant part when the operation degenerates into a negation.
> 
Jakub ACKd and I'm going to go ahead and install.  I'll pull the 
testsuite bits from a prior version.

Thanks,
Jeff

Patch hide | download patch | download mbox

Index: tree-vrp.c
===================================================================
--- tree-vrp.c	(revision 246960)
+++ tree-vrp.c	(working copy)
@@ -2461,7 +2461,20 @@  extract_range_from_binary_expr_1 (value_
 	  else if (min_op0)
 	    wmin = min_op0;
 	  else if (min_op1)
-	    wmin = minus_p ? wi::neg (min_op1) : min_op1;
+	    {
+	      if (minus_p)
+		{
+		  wmin = wi::neg (min_op1);
+
+		  /* Check for overflow.  */
+		  if (sgn == SIGNED && wi::neg_p (min_op1) && wi::neg_p (wmin))
+		    min_ovf = 1;
+		  else if (sgn == UNSIGNED && wi::ne_p (min_op1, 0))
+		    min_ovf = -1;
+		}
+	      else
+		wmin = min_op1;
+	    }
 	  else
 	    wmin = wi::shwi (0, prec);
 
@@ -2489,7 +2502,20 @@  extract_range_from_binary_expr_1 (value_
 	  else if (max_op0)
 	    wmax = max_op0;
 	  else if (max_op1)
-	    wmax = minus_p ? wi::neg (max_op1) : max_op1;
+	    {
+	      if (minus_p)
+		{
+		  wmax = wi::neg (max_op1);
+
+		  /* Check for overflow.  */
+		  if (sgn == SIGNED && wi::neg_p (max_op1) && wi::neg_p (wmax))
+		    max_ovf = 1;
+		  else if (sgn == UNSIGNED && wi::ne_p (max_op1, 0))
+		    max_ovf = -1;
+		}
+	      else
+		wmax = max_op1;
+	    }
 	  else
 	    wmax = wi::shwi (0, prec);