[PR,87059] kludge over MIN_EXPR problem causing VRP failure in value ranges

Message ID 98ec8da7-9d53-c82a-f483-bc42dc384952@redhat.com
State New
Headers show
Series
  • [PR,87059] kludge over MIN_EXPR problem causing VRP failure in value ranges
Related show

Commit Message

Aldy Hernandez Aug. 24, 2018, 4:50 p.m.
As discussed in the PR, the MIN_EXPR being passed to VRP has 
incompatible signs.  I expect MIN_EXPR to have the same type for all 
arguments plus the MIN_EXPR node itself, but this is not the case.

The culprit on PPC is expand_builtin_strncmp, but fixing it there causes 
other problems on x86-64 (see PR).  I believe Martin Sebor had some 
questions related to the x86 fallout 
(https://gcc.gnu.org/ml/gcc/2018-08/msg00164.html).

Since it seems this has been broken for a while, and I'd like to unbreak 
PPC without having to take my patch out, I suggest (for now) just 
passing the sign of the first argument as VRP had been doing all along 
(through int_const_binop):

int_const_binop():
...
   tree type = TREE_TYPE (arg1);
...
   if (TREE_CODE (arg1) == INTEGER_CST && TREE_CODE (arg2) == INTEGER_CST)
     {
       wide_int warg1 = wi::to_wide (arg1), res;
       wide_int warg2 = wi::to_wide (arg2, TYPE_PRECISION (type));
       success = wide_int_binop (res, code, warg1, warg2, sign, &overflow);
       poly_res = res;
     }

At some point later, if someone is sufficiently vexed by broken 
MIN_EXPR, we could fix it and the x86 fall out.

OK pending tests?

Comments

Jeff Law Aug. 25, 2018, 7:19 p.m. | #1
On 08/24/2018 10:50 AM, Aldy Hernandez wrote:
> As discussed in the PR, the MIN_EXPR being passed to VRP has

> incompatible signs.  I expect MIN_EXPR to have the same type for all

> arguments plus the MIN_EXPR node itself, but this is not the case.

> 

> The culprit on PPC is expand_builtin_strncmp, but fixing it there causes

> other problems on x86-64 (see PR).  I believe Martin Sebor had some

> questions related to the x86 fallout

> (https://gcc.gnu.org/ml/gcc/2018-08/msg00164.html).

> 

> Since it seems this has been broken for a while, and I'd like to unbreak

> PPC without having to take my patch out, I suggest (for now) just

> passing the sign of the first argument as VRP had been doing all along

> (through int_const_binop):

> 

> int_const_binop():

> ...

>   tree type = TREE_TYPE (arg1);

> ...

>   if (TREE_CODE (arg1) == INTEGER_CST && TREE_CODE (arg2) == INTEGER_CST)

>     {

>       wide_int warg1 = wi::to_wide (arg1), res;

>       wide_int warg2 = wi::to_wide (arg2, TYPE_PRECISION (type));

>       success = wide_int_binop (res, code, warg1, warg2, sign, &overflow);

>       poly_res = res;

>     }

> 

> At some point later, if someone is sufficiently vexed by broken

> MIN_EXPR, we could fix it and the x86 fall out.

> 

> OK pending tests?

I don't think we need this after fixing the strncmp code...

Jeff
Aldy Hernandez Aug. 25, 2018, 8:18 p.m. | #2
Agreed. Thank you Martin!

On Sat, Aug 25, 2018, 21:19 Jeff Law <law@redhat.com> wrote:

> On 08/24/2018 10:50 AM, Aldy Hernandez wrote:

> > As discussed in the PR, the MIN_EXPR being passed to VRP has

> > incompatible signs.  I expect MIN_EXPR to have the same type for all

> > arguments plus the MIN_EXPR node itself, but this is not the case.

> >

> > The culprit on PPC is expand_builtin_strncmp, but fixing it there causes

> > other problems on x86-64 (see PR).  I believe Martin Sebor had some

> > questions related to the x86 fallout

> > (https://gcc.gnu.org/ml/gcc/2018-08/msg00164.html).

> >

> > Since it seems this has been broken for a while, and I'd like to unbreak

> > PPC without having to take my patch out, I suggest (for now) just

> > passing the sign of the first argument as VRP had been doing all along

> > (through int_const_binop):

> >

> > int_const_binop():

> > ...

> >   tree type = TREE_TYPE (arg1);

> > ...

> >   if (TREE_CODE (arg1) == INTEGER_CST && TREE_CODE (arg2) == INTEGER_CST)

> >     {

> >       wide_int warg1 = wi::to_wide (arg1), res;

> >       wide_int warg2 = wi::to_wide (arg2, TYPE_PRECISION (type));

> >       success = wide_int_binop (res, code, warg1, warg2, sign,

> &overflow);

> >       poly_res = res;

> >     }

> >

> > At some point later, if someone is sufficiently vexed by broken

> > MIN_EXPR, we could fix it and the x86 fall out.

> >

> > OK pending tests?

> I don't think we need this after fixing the strncmp code...

>

> Jeff

>

>

Patch

gcc/

	PR 87059/tree-optimization
	* tree-vrp.c (extract_range_from_binary_expr_1): Pass sign of
	first argument to wide_int_range_min_max.

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 735b3646e81..7373011d901 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -1566,6 +1566,15 @@  extract_range_from_binary_expr_1 (value_range *vr,
       wide_int vr1_min, vr1_max;
       extract_range_into_wide_ints (&vr0, sign, prec, vr0_min, vr0_max);
       extract_range_into_wide_ints (&vr1, sign, prec, vr1_min, vr1_max);
+
+      /* FIXME: Currently the sign of MIN_EXPR and the sign of its
+	 arguments are not always the same.  Fixing the creators of
+	 these faulty nodes caused other problems.  For now use the
+	 sign of the first argument as VRP was previously doing.  See
+	 PR87059.  */
+      if (vr0.min)
+	sign = TYPE_SIGN (TREE_TYPE (vr0.min));
+
       if (wide_int_range_min_max (wmin, wmax, code, sign, prec,
 				  vr0_min, vr0_max, vr1_min, vr1_max))
 	set_value_range (vr, VR_RANGE,