[PR,89546] Add forgotten requeing in propagate_subaccesses_across_link

Hi,

PR 89546 is a nasty miscompilation bug that is in the compiler since
r247604 (May 2017) but is surprisingly hard to trigger.  Since that
revision, SRA does not consider all aggregates that are assigned a value
in a gimple assignment statement from another aggregate as automatically
containing data.  Instead, it records existence of such assignments and
then propagates this information from RHSs to LHSs in a simple queue
driven algorithm.  That of course relies on the fact that whenever a
part of a LHSs is marked as containing data, all relevant parts of that
aggregate which are on the RHS of some other assignment are re-queued.
Well, it turns out I forgot to add that to one spot where that has to be
done.

Fixed with the patch below.  Bootstrapped and tested on x86_64-linux.
OK for trunk and gcc 8?

Thanks,

Martin



2019-03-14  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/89546
	* tree-sra.c (propagate_subaccesses_across_link): Requeue new_acc if
	any propagation to its children took place.

	testsuite/
	* gcc.dg/tree-ssa/pr89546.c: New test.
---
 gcc/testsuite/gcc.dg/tree-ssa/pr89546.c | 100 ++++++++++++++++++++++++
 gcc/tree-sra.c                          |   8 +-
 2 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr89546.c

On Thu, 14 Mar 2019, Martin Jambor wrote:

> Hi,
> 
> PR 89546 is a nasty miscompilation bug that is in the compiler since
> r247604 (May 2017) but is surprisingly hard to trigger.  Since that
> revision, SRA does not consider all aggregates that are assigned a value
> in a gimple assignment statement from another aggregate as automatically
> containing data.  Instead, it records existence of such assignments and
> then propagates this information from RHSs to LHSs in a simple queue
> driven algorithm.  That of course relies on the fact that whenever a
> part of a LHSs is marked as containing data, all relevant parts of that
> aggregate which are on the RHS of some other assignment are re-queued.
> Well, it turns out I forgot to add that to one spot where that has to be
> done.
> 
> Fixed with the patch below.  Bootstrapped and tested on x86_64-linux.
> OK for trunk and gcc 8?

OK.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 
> 2019-03-14  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/89546
> 	* tree-sra.c (propagate_subaccesses_across_link): Requeue new_acc if
> 	any propagation to its children took place.
> 
> 	testsuite/
> 	* gcc.dg/tree-ssa/pr89546.c: New test.
> ---
>  gcc/testsuite/gcc.dg/tree-ssa/pr89546.c | 100 ++++++++++++++++++++++++
>  gcc/tree-sra.c                          |   8 +-
>  2 files changed, 106 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr89546.c
> 
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr89546.c b/gcc/testsuite/gcc.dg/tree-ssa/pr89546.c
> new file mode 100644
> index 00000000000..c4645ae1858
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr89546.c
> @@ -0,0 +1,100 @@
> +/* { dg-do run } */
> +/* { dg-options "-O" } */
> +
> +struct I
> +{
> +  int i;
> +};
> +
> +struct A
> +{
> +  struct I i1;
> +  struct I i2;
> +  struct I i3;
> +};
> +
> +struct B
> +{
> +  struct I i0;
> +  struct A a;
> +};
> +
> +struct C
> +{
> +  struct I i00;
> +  struct B b;
> +};
> +
> +volatile int v;
> +
> +void __attribute__((noipa))
> +consume_i (struct I i)
> +{
> +  v = i.i;
> +}
> +
> +void __attribute__((noipa))
> +consume_a (struct A a)
> +{
> +  v = a.i1.i;
> +}
> +
> +void __attribute__((noipa))
> +consume_b (struct B b)
> +{
> +  v = b.a.i1.i;
> +}
> +
> +void __attribute__((noipa))
> +consume_c (struct C c)
> +{
> +  v = c.b.a.i1.i;
> +}
> +
> +
> +
> +
> +int __attribute__((noipa))
> +foo (struct I input)
> +{
> +  struct I i1, i2, i3;
> +  struct A a1, a2, a3;
> +  struct B b1;
> +  struct C c;
> +
> +  i1 = input;
> +  a1.i1 = i1;
> +  b1.a = a1;
> +
> +  i2.i = 1;
> +  b1.i0 = i2;
> +
> +  c.b = b1;
> +
> +  a2 = c.b.a;
> +  a3 = a2;
> +  i3 = a3.i1;
> +
> +  int t = c.b.i0.i;
> +  a2.i3.i = 4;
> +  consume_i (i1);
> +  consume_i (i2);
> +  consume_b (b1);
> +  consume_a (a1);
> +  consume_a (a2);
> +  consume_a (a3);
> +  consume_c (c);
> +
> +  return i3.i + t;
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  struct I s;
> +  s.i = 1234;
> +  int i = foo (s);
> +  if (i != 1235)
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index ca3858d5fc7..fd51a3d0323 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -2752,8 +2752,12 @@ propagate_subaccesses_across_link (struct access *lacc, struct access *racc)
>  
>  	      rchild->grp_hint = 1;
>  	      new_acc->grp_hint |= new_acc->grp_read;
> -	      if (rchild->first_child)
> -		ret |= propagate_subaccesses_across_link (new_acc, rchild);
> +	      if (rchild->first_child
> +		  && propagate_subaccesses_across_link (new_acc, rchild))
> +		{
> +		  ret = 1;
> +		  add_access_to_work_queue (new_acc);
> +		}
>  	    }
>  	  else
>  	    {
>