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

[Richard Biener <rguenther@suse.de>]

On Thu, 17 Sep 2015, Alan Lawrence wrote:

> On 15/09/15 08:43, Richard Biener wrote:
> >
> > Sorry for chiming in so late...
> 
> Not at all, TYVM for your help!
> 
> > TREE_CONSTANT isn't the correct thing to test.  You should use
> > TREE_CODE () == INTEGER_CST instead.
> 
> Done (in some cases, via tree_fits_shwi_p).
> 
> > Also you need to handle
> > NULL_TREE TYPE_MIN/MAX_VALUE as that can happen as well.
> 
> I've not found any documentation as to what these mean, but from experiment,
> it seems that a zero-length array has MIN_VALUE 0 and MAX_VALUE null (as well
> as zero TYPE_SIZE) - so I allow that. In contrast a variable-length array also
> has zero TYPE_SIZE, but a large range of MIN-MAX, and I think I want to rule
> those out.
> 
> Another awkward case is Ada arrays with large offset (e.g. array(2^32...2^32+1)
> which has only two elements); I don't see either of tree_to_shwi or tree_to_uhwi
> as necessarily being "better" here, each will handle (some) (rare) cases the
> other will not, so I've tried to use tree_to_shwi throughout for consistency.
> 
> Summary: taken advantage of tree_fits_shwi_p, as this includes a check against
> NULL_TREE and that TREE_CODE () == INTEGER_CST.
> 
> > +      if (DECL_P (elem) && DECL_BIT_FIELD (elem))
> > +       return false;
> >
> > that can't happen (TREE_TYPE (array-type) is never a DECL).
> 
> Removed.
> 
> > +       int el_size = tree_to_uhwi (elem_size);
> > +       gcc_assert (el_size);
> >
> > so you assert on el_size being > 0 later but above you test
> > only array size ...
> 
> Good point, thanks.
> 
> > +           tree t_idx = build_int_cst (TYPE_DOMAIN (decl_type), idx);
> >
> > use t_idx = size_int (idx);
> 
> Done.
> 
> I've also added another test, of scalarizing a structure containing a
> zero-length array, as earlier attempts accidentally prevented this.
> 
> Bootstrapped + check-gcc,g++,ada,fortran on ARM and x86_64;
> Bootstrapped + check-gcc,g++,fortran on AArch64.
> 
> OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> Alan
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/67283
> 	* 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.
> 	(analyze_all_variable_accesses): Follow renamings.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/67283
> 	* gcc.dg/tree-ssa/sra-15.c: New.
> 	* gcc.dg/tree-ssa/sra-16.c: New.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/sra-15.c |  37 ++++++++
>  gcc/testsuite/gcc.dg/tree-ssa/sra-16.c |  37 ++++++++
>  gcc/tree-sra.c                         | 165 +++++++++++++++++++++++----------
>  3 files changed, 191 insertions(+), 48 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/sra-16.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..a22062e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-15.c
> @@ -0,0 +1,37 @@
> +/* Verify that SRA total scalarization works on records containing arrays.  */
> +/* { 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/testsuite/gcc.dg/tree-ssa/sra-16.c b/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
> new file mode 100644
> index 0000000..fef34c0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-16.c
> @@ -0,0 +1,37 @@
> +/* Verify that SRA total scalarization works on records containing arrays.  */
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fdump-tree-release_ssa --param sra-max-scalarization-size-Ospeed=16" } */
> +
> +extern void abort (void);
> +
> +struct S
> +{
> +  long zilch[0];
> +  char c;
> +  int i;
> +  unsigned short f3, f4;
> +};
> +
> +
> +int __attribute__ ((noinline))
> +foo (struct S *p)
> +{
> +  struct S l;
> +
> +  l = *p;
> +  l.i++;
> +  l.f3++;
> +  *p = l;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  struct S a = { { }, 0, 4, 0, 0};
> +  foo (&a);
> +  if (a.i != 5 || a.f3 != 1)
> +    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 8b3a0ad..8247554 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -915,73 +915,142 @@ 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 fixed-length
> +   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:
> +    {
> +      if (TYPE_DOMAIN (type) == NULL_TREE
> +	  || !tree_fits_shwi_p (TYPE_SIZE (type))
> +	  || !tree_fits_shwi_p (TYPE_SIZE (TREE_TYPE (type)))
> +	  || (tree_to_shwi (TYPE_SIZE (TREE_TYPE (type))) <= 0)
> +	  || !tree_fits_shwi_p (TYPE_MIN_VALUE (TYPE_DOMAIN (type))))
> +	return false;
> +      if (tree_to_shwi (TYPE_SIZE (type)) == 0
> +	  && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE)
> +	/* Zero-element array, should not prevent scalarization.  */
> +	;
> +      else if ((tree_to_shwi (TYPE_SIZE (type)) <= 0)
> +	       || !tree_fits_shwi_p (TYPE_MAX_VALUE (TYPE_DOMAIN (type))))
> +	return false;
> +
> +      tree elem = TREE_TYPE (type);
> +      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_shwi_p (elem_size));
> +	HOST_WIDE_INT el_size = tree_to_shwi (elem_size);
> +	gcc_assert (el_size > 0);
> +
> +	tree minidx = TYPE_MIN_VALUE (TYPE_DOMAIN (decl_type));
> +	gcc_assert (TREE_CODE (minidx) == INTEGER_CST);
> +	tree maxidx = TYPE_MAX_VALUE (TYPE_DOMAIN (decl_type));
> +	if (maxidx)
>  	  {
> -	    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.  */
> +	    gcc_assert (TREE_CODE (maxidx) == INTEGER_CST);
> +	    /* MINIDX and MAXIDX are inclusive.  Try to avoid overflow.  */
> +	    unsigned HOST_WIDE_INT lenp1 = tree_to_shwi (maxidx)
> +					- tree_to_shwi (minidx);
> +	    unsigned HOST_WIDE_INT idx = 0;
> +	    do
> +	      {
> +		tree nref = build4 (ARRAY_REF, elemtype, ref, size_int (idx),
> +				    NULL_TREE, NULL_TREE);
> +		int el_off = offset + idx * el_size;
> +		scalarize_elem (base, el_off, el_size, nref, elemtype);
> +	      }
> +	    while (++idx <= lenp1);
>  	  }
> -	else
> -	  completely_scalarize_record (base, fld, pos, nref);
>        }
> +      break;
> +    default:
> +      gcc_unreachable ();
> +    }
> +}
> +
> +/* Create total_scalarization accesses for a member of type TYPE, which must
> +   satisfy either is_gimple_reg_type or scalarizable_type_p.  BASE must be the
> +   top-most VAR_DECL representing the variable; within that, POS and SIZE locate
> +   the member and REF must be the reference expression for it.  */
> +
> +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 a total_scalarization access for VAR as a whole.  VAR must be of a
> -   RECORD_TYPE conforming to type_consists_of_records_p.  */
> +   RECORD_TYPE or ARRAY_TYPE conforming to scalarizable_type_p.  */
>  
>  static void
>  create_total_scalarization_access (tree var)
> @@ -2521,13 +2590,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 ");
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)