[PATCH 1/3] Hide and refactor IVOPTs strip_offset

[PATCH 1/3] Hide and refactor IVOPTs strip_offset

[Richard Biener]

PR110243 shows strip_offset has some correctness issues, the following
avoids using it from loop distribution which can use the more correct
split_constant_offset from data-ref analysis instead.  The patch then
un-exports the function and refactors it to make it obvious the
actual constant offset is only interesting in address-cases.

The series is split, removing one strip_offset at a time for easier
bisecting of code generation quality issues.

Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.

	* tree-loop-distribution.cc (classify_builtin_st): Use
	split_constant_offset.
	* tree-ssa-loop-ivopts.h (strip_offset): Remove.
	* tree-ssa-loop-ivopts.cc (strip_offset): Make static.
	Refactor to make the offset operand optional and assert
	we are dealing with addresses if its required.
---
 gcc/tree-loop-distribution.cc |  9 +++++----
 gcc/tree-ssa-loop-ivopts.cc   | 22 ++++++++++++++--------
 gcc/tree-ssa-loop-ivopts.h    |  1 -
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/gcc/tree-loop-distribution.cc b/gcc/tree-loop-distribution.cc
index 6291f941a21..cf7c197aaf7 100644
--- a/gcc/tree-loop-distribution.cc
+++ b/gcc/tree-loop-distribution.cc
@@ -1756,11 +1756,12 @@ classify_builtin_st (loop_p loop, partition *partition, data_reference_p dr)
       return;
     }
 
-  poly_uint64 base_offset;
-  unsigned HOST_WIDE_INT const_base_offset;
-  tree base_base = strip_offset (base, &base_offset);
-  if (!base_offset.is_constant (&const_base_offset))
+  tree base_offset;
+  tree base_base;
+  split_constant_offset (base, &base_base, &base_offset);
+  if (!cst_and_fits_in_hwi (base_offset))
     return;
+  unsigned HOST_WIDE_INT const_base_offset = int_cst_value (base_offset);
 
   struct builtin_info *builtin;
   builtin = alloc_builtin (dr, NULL, base, NULL_TREE, size);
diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc
index 6fbd2d59318..7978c80b39e 100644
--- a/gcc/tree-ssa-loop-ivopts.cc
+++ b/gcc/tree-ssa-loop-ivopts.cc
@@ -1175,6 +1175,9 @@ contain_complex_addr_expr (tree expr)
   return res;
 }
 
+static tree
+strip_offset (tree expr, poly_uint64_pod *offset = nullptr);
+
 /* Allocates an induction variable with given initial value BASE and step STEP
    for loop LOOP.  NO_OVERFLOW implies the iv doesn't overflow.  */
 
@@ -2942,12 +2945,16 @@ strip_offset_1 (tree expr, bool inside_addr, bool top_compref,
 
 /* Strips constant offsets from EXPR and stores them to OFFSET.  */
 
-tree
+static tree
 strip_offset (tree expr, poly_uint64_pod *offset)
 {
   poly_int64 off;
   tree core = strip_offset_1 (expr, false, false, &off);
-  *offset = off;
+  if (offset)
+    {
+      gcc_assert (POINTER_TYPE_P (expr));
+      *offset = off;
+    }
   return core;
 }
 
@@ -3512,7 +3519,6 @@ add_iv_candidate_derived_from_uses (struct ivopts_data *data)
 static void
 add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
 {
-  poly_uint64 offset;
   tree base;
   struct iv *iv = use->iv;
   tree basetype = TREE_TYPE (iv->base);
@@ -3563,8 +3569,8 @@ add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
 
   /* Record common candidate with constant offset stripped in base.
      Like the use itself, we also add candidate directly for it.  */
-  base = strip_offset (iv->base, &offset);
-  if (maybe_ne (offset, 0U) || base != iv->base)
+  base = strip_offset (iv->base);
+  if (base != iv->base)
     {
       record_common_cand (data, base, iv->step, use);
       add_candidate (data, base, iv->step, false, use);
@@ -3582,9 +3588,9 @@ add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
       step = fold_convert (sizetype, step);
       record_common_cand (data, base, step, use);
       /* Also record common candidate with offset stripped.  */
-      base = strip_offset (base, &offset);
-      if (maybe_ne (offset, 0U))
-	record_common_cand (data, base, step, use);
+      tree alt_base = strip_offset (base);
+      if (alt_base != base)
+	record_common_cand (data, alt_base, step, use);
     }
 
   /* At last, add auto-incremental candidates.  Make such variables
diff --git a/gcc/tree-ssa-loop-ivopts.h b/gcc/tree-ssa-loop-ivopts.h
index 95148616e70..7a53ce47f10 100644
--- a/gcc/tree-ssa-loop-ivopts.h
+++ b/gcc/tree-ssa-loop-ivopts.h
@@ -28,7 +28,6 @@ extern void dump_cand (FILE *, struct iv_cand *);
 extern bool contains_abnormal_ssa_name_p (tree);
 extern class loop *outermost_invariant_loop_for_expr (class loop *, tree);
 extern bool expr_invariant_in_loop_p (class loop *, tree);
-extern tree strip_offset (tree, poly_uint64_pod *);
 bool may_be_nonaddressable_p (tree expr);
 void tree_ssa_iv_optimize (void);
 
-- 
2.35.3

Re: [PATCH 1/3] Hide and refactor IVOPTs strip_offset

[Richard Biener]

On Wed, 21 Jun 2023, Richard Biener wrote:

> PR110243 shows strip_offset has some correctness issues, the following
> avoids using it from loop distribution which can use the more correct
> split_constant_offset from data-ref analysis instead.  The patch then
> un-exports the function and refactors it to make it obvious the
> actual constant offset is only interesting in address-cases.
> 
> The series is split, removing one strip_offset at a time for easier
> bisecting of code generation quality issues.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.
> 
> 	* tree-loop-distribution.cc (classify_builtin_st): Use
> 	split_constant_offset.
> 	* tree-ssa-loop-ivopts.h (strip_offset): Remove.
> 	* tree-ssa-loop-ivopts.cc (strip_offset): Make static.
> 	Refactor to make the offset operand optional and assert
> 	we are dealing with addresses if its required.

Just noticed this refactoring is a left-over and I edited it out of
this series, will push after re-bootstrapping.

Richard.

> ---
>  gcc/tree-loop-distribution.cc |  9 +++++----
>  gcc/tree-ssa-loop-ivopts.cc   | 22 ++++++++++++++--------
>  gcc/tree-ssa-loop-ivopts.h    |  1 -
>  3 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/gcc/tree-loop-distribution.cc b/gcc/tree-loop-distribution.cc
> index 6291f941a21..cf7c197aaf7 100644
> --- a/gcc/tree-loop-distribution.cc
> +++ b/gcc/tree-loop-distribution.cc
> @@ -1756,11 +1756,12 @@ classify_builtin_st (loop_p loop, partition *partition, data_reference_p dr)
>        return;
>      }
>  
> -  poly_uint64 base_offset;
> -  unsigned HOST_WIDE_INT const_base_offset;
> -  tree base_base = strip_offset (base, &base_offset);
> -  if (!base_offset.is_constant (&const_base_offset))
> +  tree base_offset;
> +  tree base_base;
> +  split_constant_offset (base, &base_base, &base_offset);
> +  if (!cst_and_fits_in_hwi (base_offset))
>      return;
> +  unsigned HOST_WIDE_INT const_base_offset = int_cst_value (base_offset);
>  
>    struct builtin_info *builtin;
>    builtin = alloc_builtin (dr, NULL, base, NULL_TREE, size);
> diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc
> index 6fbd2d59318..7978c80b39e 100644
> --- a/gcc/tree-ssa-loop-ivopts.cc
> +++ b/gcc/tree-ssa-loop-ivopts.cc
> @@ -1175,6 +1175,9 @@ contain_complex_addr_expr (tree expr)
>    return res;
>  }
>  
> +static tree
> +strip_offset (tree expr, poly_uint64_pod *offset = nullptr);
> +
>  /* Allocates an induction variable with given initial value BASE and step STEP
>     for loop LOOP.  NO_OVERFLOW implies the iv doesn't overflow.  */
>  
> @@ -2942,12 +2945,16 @@ strip_offset_1 (tree expr, bool inside_addr, bool top_compref,
>  
>  /* Strips constant offsets from EXPR and stores them to OFFSET.  */
>  
> -tree
> +static tree
>  strip_offset (tree expr, poly_uint64_pod *offset)
>  {
>    poly_int64 off;
>    tree core = strip_offset_1 (expr, false, false, &off);
> -  *offset = off;
> +  if (offset)
> +    {
> +      gcc_assert (POINTER_TYPE_P (expr));
> +      *offset = off;
> +    }
>    return core;
>  }
>  
> @@ -3512,7 +3519,6 @@ add_iv_candidate_derived_from_uses (struct ivopts_data *data)
>  static void
>  add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
>  {
> -  poly_uint64 offset;
>    tree base;
>    struct iv *iv = use->iv;
>    tree basetype = TREE_TYPE (iv->base);
> @@ -3563,8 +3569,8 @@ add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
>  
>    /* Record common candidate with constant offset stripped in base.
>       Like the use itself, we also add candidate directly for it.  */
> -  base = strip_offset (iv->base, &offset);
> -  if (maybe_ne (offset, 0U) || base != iv->base)
> +  base = strip_offset (iv->base);
> +  if (base != iv->base)
>      {
>        record_common_cand (data, base, iv->step, use);
>        add_candidate (data, base, iv->step, false, use);
> @@ -3582,9 +3588,9 @@ add_iv_candidate_for_use (struct ivopts_data *data, struct iv_use *use)
>        step = fold_convert (sizetype, step);
>        record_common_cand (data, base, step, use);
>        /* Also record common candidate with offset stripped.  */
> -      base = strip_offset (base, &offset);
> -      if (maybe_ne (offset, 0U))
> -	record_common_cand (data, base, step, use);
> +      tree alt_base = strip_offset (base);
> +      if (alt_base != base)
> +	record_common_cand (data, alt_base, step, use);
>      }
>  
>    /* At last, add auto-incremental candidates.  Make such variables
> diff --git a/gcc/tree-ssa-loop-ivopts.h b/gcc/tree-ssa-loop-ivopts.h
> index 95148616e70..7a53ce47f10 100644
> --- a/gcc/tree-ssa-loop-ivopts.h
> +++ b/gcc/tree-ssa-loop-ivopts.h
> @@ -28,7 +28,6 @@ extern void dump_cand (FILE *, struct iv_cand *);
>  extern bool contains_abnormal_ssa_name_p (tree);
>  extern class loop *outermost_invariant_loop_for_expr (class loop *, tree);
>  extern bool expr_invariant_in_loop_p (class loop *, tree);
> -extern tree strip_offset (tree, poly_uint64_pod *);
>  bool may_be_nonaddressable_p (tree expr);
>  void tree_ssa_iv_optimize (void);
>  
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)