Re: [PATCH 3/5] Build ARRAY_REFs when the base is of ARRAY_TYPE.

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Tue, Aug 25, 2015 at 12:06:15PM +0100, Alan Lawrence wrote:
> When SRA completely scalarizes an array, this patch changes the
> generated accesses from e.g.
> 
>    MEM[(int[8] *)&a + 4B] = 1;
> 
> to
> 
>    a[1] = 1;
> 
> This overcomes a limitation in dom2, that accesses to equivalent
> chunks of e.g. MEM[(int[8] *)&a] are not hashable_expr_equal_p with
> accesses to e.g. MEM[(int[8] *)&a]. This is necessary for constant
> propagation in the ssa-dom-cse-2.c testcase (after the next patch
> that makes SRA handle constant-pool loads).
> 
> I tried to work around this by making dom2's hashable_expr_equal_p
> less conservative, but found that on platforms without AArch64's
> vectorized reductions (specifically Alpha, hppa, PowerPC, and SPARC,
> mentioned in ssa-dom-cse-2.c), I also needed to make MEM[(int[8]
> *)&a] equivalent to a[0], etc.; a complete overhaul of
> hashable_expr_equal_p seems like a larger task than this patch
> series.

Uff.  Well, while I am obviously not excited about such workaround
landing in SRA, if maintainers agree that it is reasonable, I suppose
I'll manage to live with it.

I also have more specific comments:

> 
> I can't see how to write a testcase for this in C though as direct assignment to an array is not possible; such assignments occur only with constant pool data, which is dealt with in the next patch.
> 
> Bootstrap + check-gcc on x86-none-linux-gnu, arm-none-linux-gnueabihf, aarch64-none-linux-gnu.
> 
> gcc/ChangeLog:
> 
> 	* tree-sra.c (completely_scalarize): Move some code into:
> 	(get_elem_size): New.
> 	(build_ref_for_offset): Build ARRAY_REF if base is aligned array.
> ---
>  gcc/tree-sra.c | 110 ++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 69 insertions(+), 41 deletions(-)
> 
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 08fa8dc..af35fcc 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -957,6 +957,20 @@ scalarizable_type_p (tree type)
>    }
>  }
>  
> +static bool
> +get_elem_size (const_tree type, unsigned HOST_WIDE_INT *sz_out)

As Jeff already pointed out, this function needs a comment.

> +{
> +  gcc_assert (TREE_CODE (type) == ARRAY_TYPE);
> +  tree t_size = TYPE_SIZE (TREE_TYPE (type));
> +  if (!t_size || !tree_fits_uhwi_p (t_size))
> +    return false;
> +  unsigned HOST_WIDE_INT sz = tree_to_uhwi (t_size);
> +  if (!sz)
> +    return false;
> +  *sz_out = sz;
> +  return true;
> +}
> +
>  static void scalarize_elem (tree, HOST_WIDE_INT, HOST_WIDE_INT, tree, tree);
>  
>  /* Create total_scalarization accesses for all scalar fields of a member
> @@ -985,10 +999,9 @@ completely_scalarize (tree base, tree decl_type, HOST_WIDE_INT offset, tree ref)
>      case ARRAY_TYPE:
>        {
>  	tree elemtype = TREE_TYPE (decl_type);
> -	tree elem_size = TYPE_SIZE (elemtype);
> -	gcc_assert (elem_size && tree_fits_uhwi_p (elem_size));
> -	int el_size = tree_to_uhwi (elem_size);
> -	gcc_assert (el_size);
> +	unsigned HOST_WIDE_INT el_size;
> +	if (!get_elem_size (decl_type, &el_size))
> +	  gcc_assert (false);

This is usually written as gcc_unreachable ()

>  
>  	tree minidx = TYPE_MIN_VALUE (TYPE_DOMAIN (decl_type));
>  	tree maxidx = TYPE_MAX_VALUE (TYPE_DOMAIN (decl_type));
> @@ -1563,7 +1576,7 @@ build_ref_for_offset (location_t loc, tree base, HOST_WIDE_INT offset,
>    tree off;
>    tree mem_ref;
>    HOST_WIDE_INT base_offset;
> -  unsigned HOST_WIDE_INT misalign;
> +  unsigned HOST_WIDE_INT misalign, el_sz;
>    unsigned int align;
>  
>    gcc_checking_assert (offset % BITS_PER_UNIT == 0);
> @@ -1572,47 +1585,62 @@ build_ref_for_offset (location_t loc, tree base, HOST_WIDE_INT offset,
>  
>    /* get_addr_base_and_unit_offset returns NULL for references with a variable
>       offset such as array[var_index].  */
> -  if (!base)
> -    {
> -      gassign *stmt;
> -      tree tmp, addr;
> -
> -      gcc_checking_assert (gsi);
> -      tmp = make_ssa_name (build_pointer_type (TREE_TYPE (prev_base)));
> -      addr = build_fold_addr_expr (unshare_expr (prev_base));
> -      STRIP_USELESS_TYPE_CONVERSION (addr);
> -      stmt = gimple_build_assign (tmp, addr);
> -      gimple_set_location (stmt, loc);
> -      if (insert_after)
> -	gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
> -      else
> -	gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
> -
> -      off = build_int_cst (reference_alias_ptr_type (prev_base),
> -			   offset / BITS_PER_UNIT);
> -      base = tmp;
> -    }
> -  else if (TREE_CODE (base) == MEM_REF)
> -    {
> -      off = build_int_cst (TREE_TYPE (TREE_OPERAND (base, 1)),
> -			   base_offset + offset / BITS_PER_UNIT);
> -      off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1), off);
> -      base = unshare_expr (TREE_OPERAND (base, 0));
> +  if (base
> +      && TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE
> +      && misalign == 0
> +      && get_elem_size (TREE_TYPE (base), &el_sz)
> +      && ((offset % el_sz) == 0)
> +      && useless_type_conversion_p (exp_type, TREE_TYPE (TREE_TYPE (base)))
> +      && (align >= TYPE_ALIGN (exp_type)))

If this goes through, please add a comment explaining why we are doing
this, especially if you do not have a test-case.

> +    {
> +      tree idx = build_int_cst (TYPE_DOMAIN (TREE_TYPE (base)), offset / el_sz);
> +      base = unshare_expr (base);
> +      mem_ref = build4 (ARRAY_REF, exp_type, base, idx, NULL_TREE, NULL_TREE);

Since you are changing the function not to always produce a MEM_REF,
you need to adjust its comment again ;-)

Thanks,

Martin