Re: [PATCH 2/5] completely_scalarize arrays as well as records

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Tue, Aug 25, 2015 at 12:06:14PM +0100, Alan Lawrence wrote:
> This changes the completely_scalarize_record path to also work on arrays (thus
> allowing records containing arrays, etc.). This just required extending the
> existing type_consists_of_records_p and completely_scalarize_record methods
> to handle things of ARRAY_TYPE as well as RECORD_TYPE. Hence, I renamed both
> methods so as not to mention 'record'.

thanks for working on this.  I see Jeff has already approved the
patch, but I have two comments nevertheless.  First, I would be much
happier if you added a proper comment to scalarize_elem function which
you forgot completely.  The name is not very descriptive and it has
quite few parameters too.

Second, this patch should also fix PR 67283.  It would be great if you
could verify that and add it to the changelog when committing if that
is indeed the case.

Thanks,

Martin


> 
> Bootstrapped + check-gcc on aarch64-none-linux-gnu, arm-none-linux-gnueabihf and x86_64-none-linux-gnu.
> 
> Have also verified the scan-tree-dump check in the new sra-15.c passes (using a stage 1 compiler only, no execution test) on alpha, hppa, powerpc, sparc, avr and sh.
> 
> gcc/ChangeLog:
> 
> 	* tree-sra.c (type_consists_of_records_p): Rename to...
> 	(scalarizable_type_p): ...this, add case for ARRAY_TYPE.
> 
> 	(completely_scalarize_record): Rename to...
> 	(completely_scalarize): ...this, add ARRAY_TYPE case, move some code to:
> 	(scalarize_elem): New.
> 
> gcc/testsuite/ChangeLog:
> 	* gcc.dg/tree-ssa/sra-15.c: New.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/sra-15.c |  38 +++++++++
>  gcc/tree-sra.c                         | 146 ++++++++++++++++++++++-----------
>  2 files changed, 135 insertions(+), 49 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> 
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> new file mode 100644
> index 0000000..e251058
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> @@ -0,0 +1,38 @@
> +/* Verify that SRA total scalarization works on records containing arrays.  */
> +/* Test skipped for targets with small (often default) MOVE_RATIO.  */
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fdump-tree-release_ssa --param sra-max-scalarization-size-Ospeed=32" } */
> +
> +extern void abort (void);
> +
> +struct S
> +{
> +  char c;
> +  unsigned short f[2][2];
> +  int i;
> +  unsigned short f3, f4;
> +};
> +
> +
> +int __attribute__ ((noinline))
> +foo (struct S *p)
> +{
> +  struct S l;
> +
> +  l = *p;
> +  l.i++;
> +  l.f[1][0] += 3;
> +  *p = l;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  struct S a = {0, { {5, 7}, {9, 11} }, 4, 0, 0};
> +  foo (&a);
> +  if (a.i != 5 || a.f[1][0] != 12)
> +    abort ();
> +  return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "l;" 0 "release_ssa" } } */
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index a0c92b0..08fa8dc 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -915,74 +915,122 @@ create_access (tree expr, gimple stmt, bool write)
>  }
>  
>  
> -/* Return true iff TYPE is a RECORD_TYPE with fields that are either of gimple
> -   register types or (recursively) records with only these two kinds of fields.
> -   It also returns false if any of these records contains a bit-field.  */
> +/* Return true iff TYPE is scalarizable - i.e. a RECORD_TYPE or ARRAY_TYPE with
> +   fields that are either of gimple register types (excluding bit-fields)
> +   or (recursively) scalarizable types.  */
>  
>  static bool
> -type_consists_of_records_p (tree type)
> +scalarizable_type_p (tree type)
>  {
> -  tree fld;
> +  gcc_assert (!is_gimple_reg_type (type));
>  
> -  if (TREE_CODE (type) != RECORD_TYPE)
> -    return false;
> +  switch (TREE_CODE (type))
> +  {
> +  case RECORD_TYPE:
> +    for (tree fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
> +      if (TREE_CODE (fld) == FIELD_DECL)
> +	{
> +	  tree ft = TREE_TYPE (fld);
>  
> -  for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
> -    if (TREE_CODE (fld) == FIELD_DECL)
> -      {
> -	tree ft = TREE_TYPE (fld);
> +	  if (DECL_BIT_FIELD (fld))
> +	    return false;
>  
> -	if (DECL_BIT_FIELD (fld))
> -	  return false;
> +	  if (!is_gimple_reg_type (ft)
> +	      && !scalarizable_type_p (ft))
> +	    return false;
> +	}
>  
> -	if (!is_gimple_reg_type (ft)
> -	    && !type_consists_of_records_p (ft))
> -	  return false;
> -      }
> +    return true;
>  
> -  return true;
> +  case ARRAY_TYPE:
> +    {
> +      tree elem = TREE_TYPE (type);
> +      if (DECL_P (elem) && DECL_BIT_FIELD (elem))
> +	return false;
> +      if (!is_gimple_reg_type (elem)
> +	 && !scalarizable_type_p (elem))
> +	return false;
> +      return true;
> +    }
> +  default:
> +    return false;
> +  }
>  }
>  
> -/* Create total_scalarization accesses for all scalar type fields in DECL that
> -   must be of a RECORD_TYPE conforming to type_consists_of_records_p.  BASE
> -   must be the top-most VAR_DECL representing the variable, OFFSET must be the
> -   offset of DECL within BASE.  REF must be the memory reference expression for
> -   the given decl.  */
> +static void scalarize_elem (tree, HOST_WIDE_INT, HOST_WIDE_INT, tree, tree);
> +
> +/* Create total_scalarization accesses for all scalar fields of a member
> +   of type DECL_TYPE conforming to scalarizable_type_p.  BASE
> +   must be the top-most VAR_DECL representing the variable; within that,
> +   OFFSET locates the member and REF must be the memory reference expression for
> +   the member.  */
>  
>  static void
> -completely_scalarize_record (tree base, tree decl, HOST_WIDE_INT offset,
> -			     tree ref)
> +completely_scalarize (tree base, tree decl_type, HOST_WIDE_INT offset, tree ref)
>  {
> -  tree fld, decl_type = TREE_TYPE (decl);
> +  switch (TREE_CODE (decl_type))
> +    {
> +    case RECORD_TYPE:
> +      for (tree fld = TYPE_FIELDS (decl_type); fld; fld = DECL_CHAIN (fld))
> +	if (TREE_CODE (fld) == FIELD_DECL)
> +	  {
> +	    HOST_WIDE_INT pos = offset + int_bit_position (fld);
> +	    tree ft = TREE_TYPE (fld);
> +	    tree nref = build3 (COMPONENT_REF, ft, ref, fld, NULL_TREE);
>  
> -  for (fld = TYPE_FIELDS (decl_type); fld; fld = DECL_CHAIN (fld))
> -    if (TREE_CODE (fld) == FIELD_DECL)
> +	    scalarize_elem (base, pos, tree_to_uhwi (DECL_SIZE (fld)), nref,
> +			    ft);
> +	  }
> +      break;
> +    case ARRAY_TYPE:
>        {
> -	HOST_WIDE_INT pos = offset + int_bit_position (fld);
> -	tree ft = TREE_TYPE (fld);
> -	tree nref = build3 (COMPONENT_REF, TREE_TYPE (fld), ref, fld,
> -			    NULL_TREE);
> -
> -	if (is_gimple_reg_type (ft))
> +	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);
> +
> +	tree minidx = TYPE_MIN_VALUE (TYPE_DOMAIN (decl_type));
> +	tree maxidx = TYPE_MAX_VALUE (TYPE_DOMAIN (decl_type));
> +	gcc_assert (TREE_CODE (minidx) == INTEGER_CST
> +		    && TREE_CODE (maxidx) == INTEGER_CST);
> +	unsigned HOST_WIDE_INT len = tree_to_uhwi (maxidx)
> +				     + 1 - tree_to_uhwi (minidx);
> +	/* 4th operand to ARRAY_REF is size in units of the type alignment.  */
> +	for (unsigned HOST_WIDE_INT idx = 0; idx < len; idx++)
>  	  {
> -	    struct access *access;
> -	    HOST_WIDE_INT size;
> -
> -	    size = tree_to_uhwi (DECL_SIZE (fld));
> -	    access = create_access_1 (base, pos, size);
> -	    access->expr = nref;
> -	    access->type = ft;
> -	    access->grp_total_scalarization = 1;
> -	    /* Accesses for intraprocedural SRA can have their stmt NULL.  */
> +	    tree t_idx = build_int_cst (TYPE_DOMAIN (decl_type), idx);
> +	    tree nref = build4 (ARRAY_REF, elemtype, ref, t_idx, NULL_TREE,
> +				NULL_TREE);
> +	    int el_off = offset + idx * el_size;
> +	    scalarize_elem (base, el_off, el_size, nref, elemtype);
>  	  }
> -	else
> -	  completely_scalarize_record (base, fld, pos, nref);
>        }
> +      break;
> +    default:
> +      gcc_unreachable ();
> +    }
> +}
> +
> +static void
> +scalarize_elem (tree base, HOST_WIDE_INT pos, HOST_WIDE_INT size,
> +		 tree ref, tree type)
> +{
> +  if (is_gimple_reg_type (type))
> +  {
> +    struct access *access = create_access_1 (base, pos, size);
> +    access->expr = ref;
> +    access->type = type;
> +    access->grp_total_scalarization = 1;
> +    /* Accesses for intraprocedural SRA can have their stmt NULL.  */
> +  }
> +  else
> +    completely_scalarize (base, type, pos, ref);
>  }
>  
>  /* Create total_scalarization accesses for all scalar type fields in VAR and
> -   for VAR as a whole.  VAR must be of a RECORD_TYPE conforming to
> -   type_consists_of_records_p.   */
> +   for VAR as a whole.  VAR must be of a RECORD_TYPE or ARRAY_TYPE conforming to
> +   scalarizable_type_p.  */
>  
>  static void
>  create_total_scalarization_access (tree var)
> @@ -2522,13 +2570,13 @@ analyze_all_variable_accesses (void)
>  	tree var = candidate (i);
>  
>  	if (TREE_CODE (var) == VAR_DECL
> -	    && type_consists_of_records_p (TREE_TYPE (var)))
> +	    && scalarizable_type_p (TREE_TYPE (var)))
>  	  {
>  	    if (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (var)))
>  		<= max_scalarization_size)
>  	      {
>  		create_total_scalarization_access (var);
> -		completely_scalarize_record (var, var, 0, var);
> +		completely_scalarize (var, TREE_TYPE (var), 0, var);
>  		if (dump_file && (dump_flags & TDF_DETAILS))
>  		  {
>  		    fprintf (dump_file, "Will attempt to totally scalarize ");
> -- 
> 1.8.3
>