[COMMITTED] Make irange dependency explicit for range_of_ssa_name_with_loop_info.

  Even though ranger is type agnostic, SCEV seems to only work with
integers.  This patch removes some FIXME notes making it explicit that
bounds_of_var_in_loop only works with iranges.

Tested on x86-64 Linux.

gcc/ChangeLog:

	* gimple-range-fold.cc (fold_using_range::range_of_phi): Only
	query SCEV for integers.
	(fold_using_range::range_of_ssa_name_with_loop_info): Remove
	irange check.
---
 gcc/gimple-range-fold.cc | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
  

On Mon, Aug 1, 2022 at 8:17 AM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Even though ranger is type agnostic, SCEV seems to only work with
> integers.  This patch removes some FIXME notes making it explicit that
> bounds_of_var_in_loop only works with iranges.

SCEV also handles floats, where do you see this failing?  Yes, support is
somewhat limited, but still.

> Tested on x86-64 Linux.
>
> gcc/ChangeLog:
>
>         * gimple-range-fold.cc (fold_using_range::range_of_phi): Only
>         query SCEV for integers.
>         (fold_using_range::range_of_ssa_name_with_loop_info): Remove
>         irange check.
> ---
>  gcc/gimple-range-fold.cc | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
> index 6f907df5bf5..7665c954f2b 100644
> --- a/gcc/gimple-range-fold.cc
> +++ b/gcc/gimple-range-fold.cc
> @@ -853,12 +853,14 @@ fold_using_range::range_of_phi (vrange &r, gphi *phi, fur_source &src)
>        }
>
>    // If SCEV is available, query if this PHI has any knonwn values.
> -  if (scev_initialized_p () && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
> +  if (scev_initialized_p ()
> +      && !POINTER_TYPE_P (TREE_TYPE (phi_def))
> +      && irange::supports_p (TREE_TYPE (phi_def)))
>      {
> -      value_range loop_range;
>        class loop *l = loop_containing_stmt (phi);
>        if (l && loop_outer (l))
>         {
> +         int_range_max loop_range;
>           range_of_ssa_name_with_loop_info (loop_range, phi_def, l, phi, src);
>           if (!loop_range.varying_p ())
>             {
> @@ -1337,9 +1339,7 @@ fold_using_range::range_of_ssa_name_with_loop_info (irange &r, tree name,
>  {
>    gcc_checking_assert (TREE_CODE (name) == SSA_NAME);
>    tree min, max, type = TREE_TYPE (name);
> -  // FIXME: Remove the supports_p() once all this can handle floats, etc.
> -  if (irange::supports_p (type)
> -      && bounds_of_var_in_loop (&min, &max, src.query (), l, phi, name))
> +  if (bounds_of_var_in_loop (&min, &max, src.query (), l, phi, name))
>      {
>        if (TREE_CODE (min) != INTEGER_CST)
>         {
> --
> 2.37.1
>
  

On Tue, Aug 2, 2022 at 9:19 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Aug 1, 2022 at 8:17 AM Aldy Hernandez via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Even though ranger is type agnostic, SCEV seems to only work with
> > integers.  This patch removes some FIXME notes making it explicit that
> > bounds_of_var_in_loop only works with iranges.
>
> SCEV also handles floats, where do you see this failing?  Yes, support is
> somewhat limited, but still.

Oh, it does?  We could convert  range_of_ssa_name_with_loop_info to
the type agnostic vrange if so... (see attached untested patch).

I'm looking at the testcase for 24021 with the attached patch, and
bounds_of_var_in_loop() is returning false because SCEV couldn't
figure out the direction of the loop:

  dir = scev_direction (chrec);
  if (/* Do not adjust ranges if we do not know whether the iv increases
     or decreases,  ... */
      dir == EV_DIR_UNKNOWN
      /* ... or if it may wrap.  */
      || scev_probably_wraps_p (NULL_TREE, init, step, stmt,
                get_chrec_loop (chrec), true))
    return false;

The chrec in question is rather simple... a loop increasing by 0.01:

(gdb) p debug(chrec)
{1.00000000000000002081668171172168513294309377670288085938e-2, +,
1.00000001490116119384765625e-1}_1

I also see that bounds_of_var_in_loop() calls niter's
max_loop_iterations(), which if I understand things correctly, bails
at several points if the step is not integral.

I'd be happy to adapt our code, if SCEV can give me useful information.

Aldy
  

On Tue, Aug 2, 2022 at 1:41 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> On Tue, Aug 2, 2022 at 9:19 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Mon, Aug 1, 2022 at 8:17 AM Aldy Hernandez via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > Even though ranger is type agnostic, SCEV seems to only work with
> > > integers.  This patch removes some FIXME notes making it explicit that
> > > bounds_of_var_in_loop only works with iranges.
> >
> > SCEV also handles floats, where do you see this failing?  Yes, support is
> > somewhat limited, but still.
>
> Oh, it does?  We could convert  range_of_ssa_name_with_loop_info to
> the type agnostic vrange if so... (see attached untested patch).
>
> I'm looking at the testcase for 24021 with the attached patch, and
> bounds_of_var_in_loop() is returning false because SCEV couldn't
> figure out the direction of the loop:
>
>   dir = scev_direction (chrec);
>   if (/* Do not adjust ranges if we do not know whether the iv increases
>      or decreases,  ... */
>       dir == EV_DIR_UNKNOWN
>       /* ... or if it may wrap.  */
>       || scev_probably_wraps_p (NULL_TREE, init, step, stmt,
>                 get_chrec_loop (chrec), true))
>     return false;
>
> The chrec in question is rather simple... a loop increasing by 0.01:
>
> (gdb) p debug(chrec)
> {1.00000000000000002081668171172168513294309377670288085938e-2, +,
> 1.00000001490116119384765625e-1}_1

Well, yeah - I meant it "supports" floats in that it can analyze the CHRECs (you
quote that) and it shouldn't ICE anywhere.  But of course some things may
return "I don't know" when not implemented.  Like scev_direction which has

  step = CHREC_RIGHT (chrec);
  if (TREE_CODE (step) != INTEGER_CST)
    return EV_DIR_UNKNOWN;

  if (tree_int_cst_sign_bit (step))
    return EV_DIR_DECREASES;
  else
    return EV_DIR_GROWS;

so it lacks support for REAL_CST step.  Would be interesting to see what happens
with a loop with -0.0 "increment" and honoring signed zeros.  That said,
inspecting the sign bit of a REAL_CST and handling zero (of any sign) as
EV_DIR_UNKNOWN would probably work.

> I also see that bounds_of_var_in_loop() calls niter's
> max_loop_iterations(), which if I understand things correctly, bails
> at several points if the step is not integral.

Yes, a lot of code is "simplified" by not considering FP IVs.  But it "works"
in terms of "it doesn't ICE" ;)

> I'd be happy to adapt our code, if SCEV can give me useful information.
>
> Aldy
  

On Tue, Aug 2, 2022 at 2:07 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Aug 2, 2022 at 1:41 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> > On Tue, Aug 2, 2022 at 9:19 AM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Mon, Aug 1, 2022 at 8:17 AM Aldy Hernandez via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > > >
> > > > Even though ranger is type agnostic, SCEV seems to only work with
> > > > integers.  This patch removes some FIXME notes making it explicit that
> > > > bounds_of_var_in_loop only works with iranges.
> > >
> > > SCEV also handles floats, where do you see this failing?  Yes, support is
> > > somewhat limited, but still.
> >
> > Oh, it does?  We could convert  range_of_ssa_name_with_loop_info to
> > the type agnostic vrange if so... (see attached untested patch).
> >
> > I'm looking at the testcase for 24021 with the attached patch, and
> > bounds_of_var_in_loop() is returning false because SCEV couldn't
> > figure out the direction of the loop:
> >
> >   dir = scev_direction (chrec);
> >   if (/* Do not adjust ranges if we do not know whether the iv increases
> >      or decreases,  ... */
> >       dir == EV_DIR_UNKNOWN
> >       /* ... or if it may wrap.  */
> >       || scev_probably_wraps_p (NULL_TREE, init, step, stmt,
> >                 get_chrec_loop (chrec), true))
> >     return false;
> >
> > The chrec in question is rather simple... a loop increasing by 0.01:
> >
> > (gdb) p debug(chrec)
> > {1.00000000000000002081668171172168513294309377670288085938e-2, +,
> > 1.00000001490116119384765625e-1}_1
>
> Well, yeah - I meant it "supports" floats in that it can analyze the CHRECs (you
> quote that) and it shouldn't ICE anywhere.  But of course some things may
> return "I don't know" when not implemented.  Like scev_direction which has
>
>   step = CHREC_RIGHT (chrec);
>   if (TREE_CODE (step) != INTEGER_CST)
>     return EV_DIR_UNKNOWN;
>
>   if (tree_int_cst_sign_bit (step))
>     return EV_DIR_DECREASES;
>   else
>     return EV_DIR_GROWS;
>
> so it lacks support for REAL_CST step.  Would be interesting to see what happens
> with a loop with -0.0 "increment" and honoring signed zeros.  That said,
> inspecting the sign bit of a REAL_CST and handling zero (of any sign) as
> EV_DIR_UNKNOWN would probably work.
>
> > I also see that bounds_of_var_in_loop() calls niter's
> > max_loop_iterations(), which if I understand things correctly, bails
> > at several points if the step is not integral.
>
> Yes, a lot of code is "simplified" by not considering FP IVs.  But it "works"
> in terms of "it doesn't ICE" ;)

That's a very generous interpretation of "works" :-).

In that case I will make our code type agnostic, with the previously
attached patch (after testing).  Then whenever SCEV and
bounds_of_var_in_loop (which also seems to be integer centric) support
floats, everything will just work.

Thanks.
Aldy
  

On Tue, Aug 2, 2022 at 2:11 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> On Tue, Aug 2, 2022 at 2:07 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Tue, Aug 2, 2022 at 1:41 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >
> > > On Tue, Aug 2, 2022 at 9:19 AM Richard Biener
> > > <richard.guenther@gmail.com> wrote:
> > > >
> > > > On Mon, Aug 1, 2022 at 8:17 AM Aldy Hernandez via Gcc-patches
> > > > <gcc-patches@gcc.gnu.org> wrote:
> > > > >
> > > > > Even though ranger is type agnostic, SCEV seems to only work with
> > > > > integers.  This patch removes some FIXME notes making it explicit that
> > > > > bounds_of_var_in_loop only works with iranges.
> > > >
> > > > SCEV also handles floats, where do you see this failing?  Yes, support is
> > > > somewhat limited, but still.
> > >
> > > Oh, it does?  We could convert  range_of_ssa_name_with_loop_info to
> > > the type agnostic vrange if so... (see attached untested patch).
> > >
> > > I'm looking at the testcase for 24021 with the attached patch, and
> > > bounds_of_var_in_loop() is returning false because SCEV couldn't
> > > figure out the direction of the loop:
> > >
> > >   dir = scev_direction (chrec);
> > >   if (/* Do not adjust ranges if we do not know whether the iv increases
> > >      or decreases,  ... */
> > >       dir == EV_DIR_UNKNOWN
> > >       /* ... or if it may wrap.  */
> > >       || scev_probably_wraps_p (NULL_TREE, init, step, stmt,
> > >                 get_chrec_loop (chrec), true))
> > >     return false;
> > >
> > > The chrec in question is rather simple... a loop increasing by 0.01:
> > >
> > > (gdb) p debug(chrec)
> > > {1.00000000000000002081668171172168513294309377670288085938e-2, +,
> > > 1.00000001490116119384765625e-1}_1
> >
> > Well, yeah - I meant it "supports" floats in that it can analyze the CHRECs (you
> > quote that) and it shouldn't ICE anywhere.  But of course some things may
> > return "I don't know" when not implemented.  Like scev_direction which has
> >
> >   step = CHREC_RIGHT (chrec);
> >   if (TREE_CODE (step) != INTEGER_CST)
> >     return EV_DIR_UNKNOWN;
> >
> >   if (tree_int_cst_sign_bit (step))
> >     return EV_DIR_DECREASES;
> >   else
> >     return EV_DIR_GROWS;
> >
> > so it lacks support for REAL_CST step.  Would be interesting to see what happens
> > with a loop with -0.0 "increment" and honoring signed zeros.  That said,
> > inspecting the sign bit of a REAL_CST and handling zero (of any sign) as
> > EV_DIR_UNKNOWN would probably work.
> >
> > > I also see that bounds_of_var_in_loop() calls niter's
> > > max_loop_iterations(), which if I understand things correctly, bails
> > > at several points if the step is not integral.
> >
> > Yes, a lot of code is "simplified" by not considering FP IVs.  But it "works"
> > in terms of "it doesn't ICE" ;)
>
> That's a very generous interpretation of "works" :-).

;)

> In that case I will make our code type agnostic, with the previously
> attached patch (after testing).  Then whenever SCEV and
> bounds_of_var_in_loop (which also seems to be integer centric) support
> floats, everything will just work.

Sounds good.

Richard.

> Thanks.
> Aldy
>