value-range: Give up on POLY_INT_CST ranges [PR97457]

This PR shows another problem with calculating value ranges for
POLY_INT_CSTs.  We have:

  ivtmp_76 = ASSERT_EXPR <ivtmp_60, ivtmp_60 > POLY_INT_CST [9, 4294967294]>

where the VQ coefficient is unsigned but is effectively acting
as a negative number.  We wrongly give the POLY_INT_CST the range:

  [9, INT_MAX]

and things go downhill from there: later iterations of the unrolled
epilogue are wrongly removed as dead.

I guess this is the final nail in the coffin for doing VRP on
POLY_INT_CSTs.  For other similarly exotic testcases we could have
overflow for any coefficient, not just those that could be treated
as contextually negative.

Testing TYPE_OVERFLOW_UNDEFINED doesn't seem like an option because we
couldn't handle warn_strict_overflow properly.  At this stage we're
just recording a range that might or might not lead to strict-overflow
assumptions later.

It still feels like we should be able to do something here, but for
now removing the code seems safest.  It's also telling that there
are no testsuite failures on SVE from doing this.

Tested on aarch64-linux-gnu (with and without SVE) and
x86_64-linux-gnu.  OK for trunk and backports?

Richard


gcc/
	PR tree-optimization/97457
	* value-range.cc (irange::set): Don't decay POLY_INT_CST ranges
	to integer ranges.

gcc/testsuite/
	PR tree-optimization/97457
	* gcc.dg/vect/pr97457.c: New test.
---
 gcc/testsuite/gcc.dg/vect/pr97457.c | 15 +++++++++++++++
 gcc/value-range.cc                  | 30 +++++------------------------
 2 files changed, 20 insertions(+), 25 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/vect/pr97457.c

On Wed, Oct 28, 2020 at 10:44 AM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This PR shows another problem with calculating value ranges for
> POLY_INT_CSTs.  We have:
>
>   ivtmp_76 = ASSERT_EXPR <ivtmp_60, ivtmp_60 > POLY_INT_CST [9, 4294967294]>
>
> where the VQ coefficient is unsigned but is effectively acting
> as a negative number.  We wrongly give the POLY_INT_CST the range:
>
>   [9, INT_MAX]
>
> and things go downhill from there: later iterations of the unrolled
> epilogue are wrongly removed as dead.
>
> I guess this is the final nail in the coffin for doing VRP on
> POLY_INT_CSTs.  For other similarly exotic testcases we could have
> overflow for any coefficient, not just those that could be treated
> as contextually negative.
>
> Testing TYPE_OVERFLOW_UNDEFINED doesn't seem like an option because we
> couldn't handle warn_strict_overflow properly.  At this stage we're
> just recording a range that might or might not lead to strict-overflow
> assumptions later.
>
> It still feels like we should be able to do something here, but for
> now removing the code seems safest.  It's also telling that there
> are no testsuite failures on SVE from doing this.
>
> Tested on aarch64-linux-gnu (with and without SVE) and
> x86_64-linux-gnu.  OK for trunk and backports?

OK.

Richard.

> Richard
>
>
> gcc/
>         PR tree-optimization/97457
>         * value-range.cc (irange::set): Don't decay POLY_INT_CST ranges
>         to integer ranges.
>
> gcc/testsuite/
>         PR tree-optimization/97457
>         * gcc.dg/vect/pr97457.c: New test.
> ---
>  gcc/testsuite/gcc.dg/vect/pr97457.c | 15 +++++++++++++++
>  gcc/value-range.cc                  | 30 +++++------------------------
>  2 files changed, 20 insertions(+), 25 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/vect/pr97457.c
>
> diff --git a/gcc/testsuite/gcc.dg/vect/pr97457.c b/gcc/testsuite/gcc.dg/vect/pr97457.c
> new file mode 100644
> index 00000000000..506ba249b00
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vect/pr97457.c
> @@ -0,0 +1,15 @@
> +/* { dg-additional-options "-O3" } */
> +
> +int a;
> +long c;
> +signed char d(char e, char f) { return e + f; }
> +int main(void) {
> +  for (; a <= 1; a++) {
> +    c = -8;
> +    for (; c != 3; c = d(c, 1))
> +      ;
> +  }
> +  char b = c;
> +  if (b != 3)
> +    __builtin_abort();
> +}
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index 7847104050c..2319c13388a 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -248,31 +248,11 @@ irange::set (tree min, tree max, value_range_kind kind)
>        set_undefined ();
>        return;
>      }
> -  if (kind == VR_RANGE)
> -    {
> -      /* Convert POLY_INT_CST bounds into worst-case INTEGER_CST bounds.  */
> -      if (POLY_INT_CST_P (min))
> -       {
> -         tree type_min = vrp_val_min (TREE_TYPE (min));
> -         widest_int lb
> -           = constant_lower_bound_with_limit (wi::to_poly_widest (min),
> -                                              wi::to_widest (type_min));
> -         min = wide_int_to_tree (TREE_TYPE (min), lb);
> -       }
> -      if (POLY_INT_CST_P (max))
> -       {
> -         tree type_max = vrp_val_max (TREE_TYPE (max));
> -         widest_int ub
> -           = constant_upper_bound_with_limit (wi::to_poly_widest (max),
> -                                              wi::to_widest (type_max));
> -         max = wide_int_to_tree (TREE_TYPE (max), ub);
> -       }
> -    }
> -  else if (kind != VR_VARYING)
> -    {
> -     if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))
> -       kind = VR_VARYING;
> -    }
> +
> +  if (kind != VR_VARYING
> +      && (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)))
> +    kind = VR_VARYING;
> +
>    if (kind == VR_VARYING)
>      {
>        set_varying (TREE_TYPE (min));

On 10/28/20 5:43 AM, Richard Sandiford via Gcc-patches wrote:
> This PR shows another problem with calculating value ranges for
> POLY_INT_CSTs.  We have:
>
>    ivtmp_76 = ASSERT_EXPR <ivtmp_60, ivtmp_60 > POLY_INT_CST [9, 4294967294]>
>
> where the VQ coefficient is unsigned but is effectively acting
> as a negative number.  We wrongly give the POLY_INT_CST the range:
>
>    [9, INT_MAX]
>
> and things go downhill from there: later iterations of the unrolled
> epilogue are wrongly removed as dead.
>
> I guess this is the final nail in the coffin for doing VRP on
> POLY_INT_CSTs.  For other similarly exotic testcases we could have
> overflow for any coefficient, not just those that could be treated
> as contextually negative.
>
> Testing TYPE_OVERFLOW_UNDEFINED doesn't seem like an option because we
> couldn't handle warn_strict_overflow properly.  At this stage we're
> just recording a range that might or might not lead to strict-overflow
> assumptions later.
>
> It still feels like we should be able to do something here, but for
> now removing the code seems safest.  It's also telling that there
> are no testsuite failures on SVE from doing this.

ONce things calm down and I get some time, Im happy to see if there is 
some way we can integrate POLYINTs better with irange going forward.  I 
don't understand them very well right now, and have some other things on 
my plate that I need to get to first.

Andrew