On Wed, Oct 21, 2020 at 9:30 AM Aldy Hernandez <al...@redhat.com> wrote:
>
>
>
> 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?
There are no legitimate uses in GIMPLE. They are (ab-)used by
GENERIC folding for propagating overflow (also used in FE
diagnostics). Generally the better way is to use wide_ints overflow
handling which also "sticks".
> >
> > 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?
Ah, that's an oversight here. And yes, "fixing" it in scalar evolution
analysis itself (dropping the flag there) would be best
>
> If the latter, we could instead what I do below. What do you think?
Yeah, though *cough* goto ... (well, not so bad I guess)
Thanks,
Richard.
> 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. */
>
