Re: [PATCH] use get_range_query to replace get_global_range_query

[Andrew Pinski <pinskia@gmail.com>]

On Tue, Oct 10, 2023 at 12:02 AM Richard Biener <rguenther@suse.de> wrote:
>
> On Tue, 10 Oct 2023, Jiufu Guo wrote:
>
> > Hi,
> >
> > For "get_global_range_query" SSA_NAME_RANGE_INFO can be queried.
> > For "get_range_query", it could get more context-aware range info.
> > And look at the implementation of "get_range_query",  it returns
> > global range if no local fun info.
> >
> > So, if not quering for SSA_NAME, it would be ok to use get_range_query
> > to replace get_global_range_query.
> >
> > Patch https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630389.html,
> > Uses get_range_query could handle more cases.
> >
> > This patch replaces get_global_range_query by get_range_query for
> > most possible code pieces (but deoes not draft new test cases).
> >
> > Pass bootstrap & regtest on ppc64{,le} and x86_64.
> > Is this ok for trunk.
>
> See below
>
> >
> > BR,
> > Jeff (Jiufu Guo)
> >
> > gcc/ChangeLog:
> >
> >       * builtins.cc (expand_builtin_strnlen): Replace get_global_range_query
> >       by get_range_query.
> >       * fold-const.cc (expr_not_equal_to): Likewise.
> >       * gimple-fold.cc (size_must_be_zero_p): Likewise.
> >       * gimple-range-fold.cc (fur_source::fur_source): Likewise.
> >       * gimple-ssa-warn-access.cc (check_nul_terminated_array): Likewise.
> >       * tree-dfa.cc (get_ref_base_and_extent): Likewise.
> >       * tree-ssa-loop-split.cc (split_at_bb_p): Likewise.
> >       * tree-ssa-loop-unswitch.cc (evaluate_control_stmt_using_entry_checks):
> >       Likewise.
> >
> > ---
> >  gcc/builtins.cc               | 2 +-
> >  gcc/fold-const.cc             | 6 +-----
> >  gcc/gimple-fold.cc            | 6 ++----
> >  gcc/gimple-range-fold.cc      | 4 +---
> >  gcc/gimple-ssa-warn-access.cc | 2 +-
> >  gcc/tree-dfa.cc               | 5 +----
> >  gcc/tree-ssa-loop-split.cc    | 2 +-
> >  gcc/tree-ssa-loop-unswitch.cc | 2 +-
> >  8 files changed, 9 insertions(+), 20 deletions(-)
> >
> > diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> > index cb90bd03b3e..4e0a77ff8e0 100644
> > --- a/gcc/builtins.cc
> > +++ b/gcc/builtins.cc
> > @@ -3477,7 +3477,7 @@ expand_builtin_strnlen (tree exp, rtx target, machine_mode target_mode)
> >
> >    wide_int min, max;
> >    value_range r;
> > -  get_global_range_query ()->range_of_expr (r, bound);
> > +  get_range_query (cfun)->range_of_expr (r, bound);
>
> expand doesn't have a ranger instance so this is a no-op.  I'm unsure
> if it would be safe given we're half GIMPLE, half RTL.  Please leave it
> out.

It definitely does not work and can't as I tried to enable a ranger
instance and it didn't work. I wrote up my experience here:
https://gcc.gnu.org/pipermail/gcc/2023-September/242407.html

Thanks,
Andrew Pinski

>
> >    if (r.varying_p () || r.undefined_p ())
> >      return NULL_RTX;
> >    min = r.lower_bound ();
> > diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> > index 4f8561509ff..15134b21b9f 100644
> > --- a/gcc/fold-const.cc
> > +++ b/gcc/fold-const.cc
> > @@ -11056,11 +11056,7 @@ expr_not_equal_to (tree t, const wide_int &w)
> >        if (!INTEGRAL_TYPE_P (TREE_TYPE (t)))
> >       return false;
> >
> > -      if (cfun)
> > -     get_range_query (cfun)->range_of_expr (vr, t);
> > -      else
> > -     get_global_range_query ()->range_of_expr (vr, t);
> > -
> > +      get_range_query (cfun)->range_of_expr (vr, t);
>
> These kind of changes look obvious.
>
> >        if (!vr.undefined_p () && !vr.contains_p (w))
> >       return true;
> >        /* If T has some known zero bits and W has any of those bits set,
> > diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> > index dc89975270c..853edd9e5d4 100644
> > --- a/gcc/gimple-fold.cc
> > +++ b/gcc/gimple-fold.cc
> > @@ -876,10 +876,8 @@ size_must_be_zero_p (tree size)
> >    wide_int zero = wi::zero (TYPE_PRECISION (type));
> >    value_range valid_range (type, zero, ssize_max);
> >    value_range vr;
> > -  if (cfun)
> > -    get_range_query (cfun)->range_of_expr (vr, size);
> > -  else
> > -    get_global_range_query ()->range_of_expr (vr, size);
> > +  get_range_query (cfun)->range_of_expr (vr, size);
> > +
> >    if (vr.undefined_p ())
> >      vr.set_varying (TREE_TYPE (size));
> >    vr.intersect (valid_range);
> > diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
> > index d1945ccb554..6e9530c3d7f 100644
> > --- a/gcc/gimple-range-fold.cc
> > +++ b/gcc/gimple-range-fold.cc
> > @@ -50,10 +50,8 @@ fur_source::fur_source (range_query *q)
> >  {
> >    if (q)
> >      m_query = q;
> > -  else if (cfun)
> > -    m_query = get_range_query (cfun);
> >    else
> > -    m_query = get_global_range_query ();
> > +    m_query = get_range_query (cfun);
> >    m_gori = NULL;
> >  }
> >
> > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> > index fcaff128d60..e439d1b9b68 100644
> > --- a/gcc/gimple-ssa-warn-access.cc
> > +++ b/gcc/gimple-ssa-warn-access.cc
> > @@ -332,7 +332,7 @@ check_nul_terminated_array (GimpleOrTree expr, tree src, tree bound)
> >      {
> >        Value_Range r (TREE_TYPE (bound));
> >
> > -      get_global_range_query ()->range_of_expr (r, bound);
> > +      get_range_query (cfun)->range_of_expr (r, bound);
> >
> >        if (r.undefined_p () || r.varying_p ())
> >       return true;
>
> The pass has a ranger instance, so yes, this should improve things.
> Since the pass doesn't do any IL modification it should also be safe.
>
> > diff --git a/gcc/tree-dfa.cc b/gcc/tree-dfa.cc
> > index af8e9243947..5355af2c869 100644
> > --- a/gcc/tree-dfa.cc
> > +++ b/gcc/tree-dfa.cc
> > @@ -531,10 +531,7 @@ get_ref_base_and_extent (tree exp, poly_int64 *poffset,
> >
> >               value_range vr;
> >               range_query *query;
> > -             if (cfun)
> > -               query = get_range_query (cfun);
> > -             else
> > -               query = get_global_range_query ();
> > +             query = get_range_query (cfun);
> >
> >               if (TREE_CODE (index) == SSA_NAME
> >                   && (low_bound = array_ref_low_bound (exp),
> > diff --git a/gcc/tree-ssa-loop-split.cc b/gcc/tree-ssa-loop-split.cc
> > index 64464802c1e..e85a1881526 100644
> > --- a/gcc/tree-ssa-loop-split.cc
> > +++ b/gcc/tree-ssa-loop-split.cc
> > @@ -145,7 +145,7 @@ split_at_bb_p (class loop *loop, basic_block bb, tree *border, affine_iv *iv,
> >       else
> >         {
> >           int_range<2> r;
> > -         get_global_range_query ()->range_of_expr (r, op0, stmt);
> > +         get_range_query (cfun)->range_of_expr (r, op0, stmt);
>
> loop splitting doesn't have a ranger instance so this is a no-op change
> but I'm also not sure it would be safe to use a dynamic ranger instance
> here since we are doing even CFG manipulations between.  Please leave
> this change out.
>
> >           if (!r.varying_p () && !r.undefined_p ()
> >               && TREE_CODE (op1) == INTEGER_CST)
> >             {
> > diff --git a/gcc/tree-ssa-loop-unswitch.cc b/gcc/tree-ssa-loop-unswitch.cc
> > index 619b50fb4bb..b3dc2ded931 100644
> > --- a/gcc/tree-ssa-loop-unswitch.cc
> > +++ b/gcc/tree-ssa-loop-unswitch.cc
> > @@ -764,7 +764,7 @@ evaluate_control_stmt_using_entry_checks (gimple *stmt,
> >
> >         int_range_max r;
> >         if (!ranger->gori ().outgoing_edge_range_p (r, e, idx,
> > -                                                   *get_global_range_query ()))
> > +                                                   *get_range_query (cfun)))
> >           continue;
>
> unswitching has a ranger instance but it does perform IL modification.
> Did you check whether the use of the global ranger was intentional here?
> Specifically we do have the 'ranger' object here and IIRC using global
> ranges was intentional.  So please leave this change out.
>
> Thanks,
> Richard.
>
> >         r.intersect (path_range);
> >         if (r.undefined_p ())
> >
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH,
> Frankenstrasse 146, 90461 Nuernberg, Germany;
> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)