Re: [PATCH] use get_range_query to replace get_global_range_query

[Richard Biener <rguenther@suse.de>]

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.

>    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)