* Re: [PATCH] use get_range_query to replace get_global_range_query
2023-10-10 7:01 ` Richard Biener
@ 2023-10-10 7:23 ` Andrew Pinski
2023-10-10 9:07 ` Jiufu Guo
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Pinski @ 2023-10-10 7:23 UTC (permalink / raw)
To: Richard Biener
Cc: Jiufu Guo, gcc-patches, jeffreyalaw, richard.sandiford, segher,
dje.gcc, linkw, bergner, amacleod, aldyh
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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] use get_range_query to replace get_global_range_query
2023-10-10 7:01 ` Richard Biener
2023-10-10 7:23 ` Andrew Pinski
@ 2023-10-10 9:07 ` Jiufu Guo
1 sibling, 0 replies; 4+ messages in thread
From: Jiufu Guo @ 2023-10-10 9:07 UTC (permalink / raw)
To: Richard Biener
Cc: gcc-patches, jeffreyalaw, richard.sandiford, segher, dje.gcc,
linkw, bergner, amacleod, aldyh
Hi,
Richard Biener <rguenther@suse.de> writes:
> 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
Thanks so much for your quick review!
>
>>
>> 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.
Oh, yeap. There is no local ranger, and 'bound' is SSA_NAME,
and SSA_NAME_RANGE_INFO is there.
get_global_range_query should be used.
>
>> 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.
Yes.
>
>> 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.
Oh, yes, get_global_range_query would be prefer here.
>
>> 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 for pointing this out!
BR,
Jeff (Jiufu Guo)
>
> Thanks,
> Richard.
>
>> r.intersect (path_range);
>> if (r.undefined_p ())
>>
^ permalink raw reply [flat|nested] 4+ messages in thread