| 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 | expand |
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
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 > >
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,
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?