Re: [PR 80898] Propagate grp_write from disqualified SRA candidates

[Richard Biener <rguenther at suse dot de>]

On Thu, 1 Jun 2017, Martin Jambor wrote:

> Hi,
> 
> when I wrote the lazy setting of grp_write flag early next year, I
> made a mistake when thinking about what to do about SRA candidates
> that were disqualified but form a RHS of an assignment link which was
> to be used to set grp_write of the LHS when appropriate.  The code
> expects that the RHS accesses form an access tree, but given that some
> are rejected exactly because such a tree cannot be built, it does not
> work.
> 
> The solution is to move dealing with disqualified RHSs to the
> assignment link processing.  The patch below checks RHS and if it is
> disqualified, marks the corresponding LHS as containing data.  As the
> second testcase shows, that information must be then also propagated
> downwards (this is not necessary in the normal propagation case
> because there propagate_subaccesses_across_link will already do that
> more elaborately) as well as upwards.
> 
> Bootstrapped and tested on x86_64-linux without any issues. OK for
> trunk?

Ok.

Thanks,
Richard.

> 
> Thanks,
> 
> Martin
> 
> 
> 2017-06-01  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/80898
> 	* tree-sra.c (process_subtree_disqualification): Removed.
> 	(disqualify_candidate): Do not call
> 	process_subtree_disqualification.
> 	(subtree_mark_written_and_enqueue): New function.
> 	(propagate_all_subaccesses): Set grp_write of LHS subtree if the
> 	RHS has been disqualified and re-queue LHS if necessary.  Apart
> 	from that, ignore disqualified RHS.
> 
> testsuite/
> 	* gcc.dg/tree-ssa/pr80898.c: New test.
> 	* gcc.dg/tree-ssa/pr80898-2.c: Likewise.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/pr80898-2.c | 71 +++++++++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/tree-ssa/pr80898.c   | 20 +++++++++
>  gcc/tree-sra.c                            | 56 +++++++++++++++---------
>  3 files changed, 126 insertions(+), 21 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr80898-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr80898.c
> 
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80898-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80898-2.c
> new file mode 100644
> index 00000000000..cb4799c5ced
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80898-2.c
> @@ -0,0 +1,71 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +struct S0
> +{
> +  unsigned a : 15;
> +  int b;
> +  int c;
> +};
> +
> +struct S1
> +{
> +  struct S0 s0;
> +  int e;
> +};
> +
> +struct Z
> +{
> +  char c;
> +  int z;
> +} __attribute__((packed));
> +
> +union U
> +{
> +  struct S1 s1;
> +  struct Z z;
> +};
> +
> +
> +int __attribute__((noinline, noclone))
> +return_zero (void)
> +{
> +  return 0;
> +}
> +
> +volatile union U gu;
> +struct S0 gs;
> +
> +int __attribute__((noinline, noclone))
> +check_outcome ()
> +{
> +  if (gs.a != 6
> +      || gs.b != 80000)
> +    __builtin_abort ();
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  union U u;
> +  struct S1 m;
> +  struct S0 l;
> +
> +  if (return_zero ())
> +    u.z.z = 20000;
> +  else
> +    {
> +      u.s1.s0.a = 6;
> +      u.s1.s0.b = 80000;
> +      u.s1.e = 2;
> +
> +      m = u.s1;
> +      m.s0.c = 0;
> +      l = m.s0;
> +      gs = l;
> +    }
> +
> +  gu = u;
> +  check_outcome ();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80898.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80898.c
> new file mode 100644
> index 00000000000..ed88f2cbd1a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80898.c
> @@ -0,0 +1,20 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +struct S0 {
> +  int f0 : 24;
> +  int f1;
> +  int f74;
> +} a, *c = &a;
> +struct S0 fn1() {
> +  struct S0 b = {4, 3};
> +  return b;
> +}
> +
> +int main() {
> +  *c = fn1();
> +
> +  if (a.f1 != 3)
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 6a8a0a4a427..f25818f4481 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -694,21 +694,9 @@ static bool constant_decl_p (tree decl)
>    return VAR_P (decl) && DECL_IN_CONSTANT_POOL (decl);
>  }
>  
> -
> -/* Mark LHS of assign links out of ACCESS and its children as written to.  */
> -
> -static void
> -process_subtree_disqualification (struct access *access)
> -{
> -  struct access *child;
> -  for (struct assign_link *link = access->first_link; link; link = link->next)
> -    link->lacc->grp_write = true;
> -  for (child = access->first_child; child; child = child->next_sibling)
> -    process_subtree_disqualification (child);
> -}
> -
>  /* Remove DECL from candidates for SRA and write REASON to the dump file if
>     there is one.  */
> +
>  static void
>  disqualify_candidate (tree decl, const char *reason)
>  {
> @@ -723,13 +711,6 @@ disqualify_candidate (tree decl, const char *reason)
>        print_generic_expr (dump_file, decl);
>        fprintf (dump_file, " - %s\n", reason);
>      }
> -
> -  struct access *access = get_first_repr_for_decl (decl);
> -  while (access)
> -    {
> -      process_subtree_disqualification (access);
> -      access = access->next_grp;
> -    }
>  }
>  
>  /* Return true iff the type contains a field or an element which does not allow
> @@ -2679,6 +2660,26 @@ propagate_subaccesses_across_link (struct access *lacc, struct access *racc)
>    return ret;
>  }
>  
> +/* Beginning with ACCESS, traverse its whole access subtree and mark all
> +   sub-trees as written to.  If any of them has not been marked so previously
> +   and has assignment links leading from it, re-enqueue it.  */
> +
> +static void
> +subtree_mark_written_and_enqueue (struct access *access)
> +{
> +  if (access->grp_write)
> +    return;
> +  access->grp_write = true;
> +  if (access->first_link)
> +    add_access_to_work_queue (access);
> +
> +  struct access *child;
> +  for (child = access->first_child; child; child = child->next_sibling)
> +    subtree_mark_written_and_enqueue (child);
> +}
> +
> +
> +
>  /* Propagate all subaccesses across assignment links.  */
>  
>  static void
> @@ -2698,7 +2699,20 @@ propagate_all_subaccesses (void)
>  	  if (!bitmap_bit_p (candidate_bitmap, DECL_UID (lacc->base)))
>  	    continue;
>  	  lacc = lacc->group_representative;
> -	  if (propagate_subaccesses_across_link (lacc, racc))
> +
> +	  bool reque_parents = false;
> +	  if (!bitmap_bit_p (candidate_bitmap, DECL_UID (racc->base)))
> +	    {
> +	      if (!lacc->grp_write)
> +		{
> +		  subtree_mark_written_and_enqueue (lacc);
> +		  reque_parents = true;
> +		}
> +	    }
> +	  else if (propagate_subaccesses_across_link (lacc, racc))
> +	    reque_parents = true;
> +
> +	  if (reque_parents)
>  	    do
>  	      {
>  		if (lacc->first_link)
> 

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