Re: [PATCH] Saturate overflows return from SCEV in ranger.

[Aldy Hernandez via Gcc-patches]

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.  */