[PATCH] Saturate overflows return from SCEV in ranger.

[Aldy Hernandez]

On 10/21/20 8:19 AM, Richard Biener wrote:
> On Tue, Oct 20, 2020 at 5:21 PM Aldy Hernandez via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> bounds_of_var_in_loop is returning an overflowed int, which is causing
>> us to create a range for which we can't compare the bounds causing
>> an ICE in verify_range.
>>
>> Overflowed bounds cause compare_values() to return -2, which we
>> don't handle in verify_range.
>>
>> We don't represent overflowed ranges in irange, so this patch just
>> saturates any overflowed end-points to MIN or MAX.
> 
> I don't think TREE_OVERFLOW means what you think it means in the
> context of bounds_of_var_in_loop - look at its bottom which does
> 
>    /* Even for valid range info, sometimes overflow flag will leak in.
>       As GIMPLE IL should have no constants with TREE_OVERFLOW set, we
>       drop them.  */
>    if (TREE_OVERFLOW_P (*min))
>      *min = drop_tree_overflow (*min);
>    if (TREE_OVERFLOW_P (*max))
>      *max = drop_tree_overflow (*max);

Interesting.

If these values "leaked" in.  Should they have been fixed at the source, 
instead of after the fact?  You mention below that every use of 
TREE_OVERFLOW in the ME is a bug, should we clean them up before 
arriving in gimple, or are there legitimate uses of it?

> 
> and the code explicitly checks for overflow, doing range adjustments
> accordingly.

Well, not all overflows are adjusted:

   /* Like in PR19590, scev can return a constant function.  */
   if (is_gimple_min_invariant (chrec))
     {
       *min = *max = chrec;
       return true;
     }

Are these min/max not adjusted for overflow by design, or is this an 
oversight?

If the latter, we could instead what I do below.  What do you think?

Thanks for the feedback.
Aldy

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index b790d62d75f..c5520e0700b 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -1156,9 +1156,9 @@ gimple_ranger::range_of_ssa_name_with_loop_info 
(irange &r, tree name,
        // ?? We could do better here.  Since MIN/MAX can only be an
        // SSA, SSA +- INTEGER_CST, or INTEGER_CST, we could easily call
        // the ranger and solve anything not an integer.
-      if (TREE_CODE (min) != INTEGER_CST || TREE_OVERFLOW (min))
+      if (TREE_CODE (min) != INTEGER_CST)
  	min = vrp_val_min (type);
-      if (TREE_CODE (max) != INTEGER_CST || TREE_OVERFLOW (max))
+      if (TREE_CODE (max) != INTEGER_CST)
  	max = vrp_val_max (type);
        r.set (min, max);
      }
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 67c88006f13..7778ceccf0a 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -1844,7 +1844,7 @@ bounds_of_var_in_loop (tree *min, tree *max, 
range_query *query,
    if (is_gimple_min_invariant (chrec))
      {
        *min = *max = chrec;
-      return true;
+      goto fix_overflow;
      }

    if (TREE_CODE (chrec) != POLYNOMIAL_CHREC)
@@ -1964,6 +1964,7 @@ bounds_of_var_in_loop (tree *min, tree *max, 
range_query *query,
    else
      *min = init;

+ fix_overflow:
    /* Even for valid range info, sometimes overflow flag will leak in.
       As GIMPLE IL should have no constants with TREE_OVERFLOW set, we
       drop them.  */