Fix 44914 - add TODO_cleanup_cfg to IPA-SRA

Hi,

this simple patch fixes PR 44914.  IPA-SRA might have left some
unreachable BBs in the CFG.

Bootstrapped and regression tested without any problems on both trunk
and the 4.5 branch.  I would like to commit it to both but I
understand the 4.5 branch is frozen.  OTOH, this patch is simple and
it would be nice to have it in 4.5.1.  Richi, please let me know what
I should do.

Thanks,

Martin



2010-07-22  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/44914
	* tree-sra.c (ipa_early_sra): Return also TODO_cleanup_cfg if function
	was changed.

	* testsuite/g++.dg/tree-ssa/pr44914.C: New test.

On Thu, Jul 22, 2010 at 8:06 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> this simple patch fixes PR 44914.  IPA-SRA might have left some
> unreachable BBs in the CFG.

If it's only unreachable BBs, then you'll do better to just call
delete_unreachable_blocks at the end of SRA, if something changed.
Better still, if you can do it only if something changed that may make
BBs unreachable. That is the common case, otherwise this bug would
have been found much sooner :-)

Ciao!
Steven

On Thu, 22 Jul 2010, Martin Jambor wrote:

> Hi,
> 
> this simple patch fixes PR 44914.  IPA-SRA might have left some
> unreachable BBs in the CFG.

Hm.  The usual pattern for eh cleanup is

  if (maybe_clean_eh_stmt (stmt)
      && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
    cfg_changed = true;

and if the cfg changed add TODO_cleanup_cfg to the TODO.  I see
that tree-sra misses gimple_purge_dead_eh_edges on a path where
it does maybe_clean_eh_stmt and it doesn't call it conditionally.

(note that if maybe_clean_eh_stmt returns true we know it is the
last stmt in the block, so the bb_changed flag isn't necessary in
ipa_sra_modify_function_body).  Both sra_modify_function_body and
ipa_sra_modify_function_body should return whether they changed
the CFG and propagate that flag so TODO_cleanup_cfg is added.

Can you rework the patch this way?

Thanks,
Richard.

> Bootstrapped and regression tested without any problems on both trunk
> and the 4.5 branch.  I would like to commit it to both but I
> understand the 4.5 branch is frozen.  OTOH, this patch is simple and
> it would be nice to have it in 4.5.1.  Richi, please let me know what
> I should do.
> 
> Thanks,
> 
> Martin
> 
> 
> 
> 2010-07-22  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/44914
> 	* tree-sra.c (ipa_early_sra): Return also TODO_cleanup_cfg if function
> 	was changed.
> 
> 	* testsuite/g++.dg/tree-ssa/pr44914.C: New test.
> 
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -4410,7 +4410,7 @@ ipa_early_sra (void)
>  
>    modify_function (node, adjustments);
>    VEC_free (ipa_parm_adjustment_t, heap, adjustments);
> -  ret = TODO_update_ssa;
> +  ret = TODO_update_ssa | TODO_cleanup_cfg;
>  
>    statistics_counter_event (cfun, "Unused parameters deleted",
>  			    sra_stats.deleted_unused_parameters);
> Index: mine/gcc/testsuite/g++.dg/tree-ssa/pr44914.C
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/g++.dg/tree-ssa/pr44914.C
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fipa-sra -fnon-call-exceptions" } */
> +
> +struct A
> +{
> +  ~A () { }
> +};
> +
> +struct B
> +{
> +  A a;
> +  int i;
> +  void f (int) { }
> +  B ()
> +  {
> +    f (i);
> +  }
> +};
> +
> +B b;
> 
>