This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix ipa-split handling of clobbers and debug stmts in return_bb (PR ipa/68672)


On Thu, 11 Feb 2016, Jakub Jelinek wrote:

> Hi!
> 
> While the cgraph versioning code is able to deal with clobber stmts that
> refer to SSA_NAMEs set by basic blocks not split into the versioned
> function, and similarly with debug stmts (clobbers refererring to those
> are removed and debug stmts reset), on the main part that doesn't happen,
> because ipa-split makes the split blocks unreachable by jumping around them
> and thus if SSA_NAMEs set in the split basic blocks are referenced
> in return_bb in statements we intentionally ignored for the analysis (debug
> stmts and clobber stmts), we either end up with weird stuff (set debug stmts
> to debug temporary that is set in the basic blocks that are removed as
> dead), or for clobbers ICE.
> 
> Richard has tried to deal with this by throwing away the return_bb in
> certain cases in the main part (forcing recreation of it), but that can
> throw valuable statements that we could have kept (debug stmts or e.g. decl
> clobbers or clobbers that don't need SSA_NAMEs from the split blocks),
> and doesn't work e.g. on the 1st and 3rd testcase below anyway.
> 
> So, this patch reverts the main_part_return_p and instead does
> what is needed for those ignored stmts in return_bb:
> 1) SSA_NAME references to retval in both debug stmts and clobber stmts
>    are replaced with the lhs of the call to *.part.*
> 2) debug stmts referencing other SSA_NAMEs set in the split bbs are reset
> 3) clobber stmts referencing other SSA_NAMEs set in the split bbs are removed
> 
> The testcase hopefully cover all the interesting cases (return_bb copied
> from main part (where it is kept) to split part too, and with debug stmts
> and clobbers that refer to SSA_NAMEs set in main or split part or retval).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2016-02-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR ipa/68672
> 	* ipa-split.c (split_function): Don't compute/use main_part_return_p.
> 	Compute retval and retbnd early in all cases if split_part_return_p
> 	and return_bb is not EXIT.  Remove all clobber stmts and reset
> 	all debug stmts that refer to SSA_NAMEs defined in split part,
> 	except if it is retval, in that case replace the old retval with the
> 	lhs of the call to the split part.
> 
> 	* g++.dg/ipa/pr68672-1.C: New test.
> 	* g++.dg/ipa/pr68672-2.C: New test.
> 	* g++.dg/ipa/pr68672-3.C: New test.
> 
> --- gcc/ipa-split.c.jj	2016-02-11 10:50:52.888220581 +0100
> +++ gcc/ipa-split.c	2016-02-11 12:46:15.975777652 +0100
> @@ -1244,28 +1244,13 @@ split_function (basic_block return_bb, s
>  	args_to_pass.safe_push (arg);
>        }
>  
> -  /* See if the split function or the main part will return.  */
> -  bool main_part_return_p = false;
> +  /* See if the split function will return.  */
>    bool split_part_return_p = false;
>    FOR_EACH_EDGE (e, ei, return_bb->preds)
>      {
>        if (bitmap_bit_p (split_point->split_bbs, e->src->index))
>  	split_part_return_p = true;
> -      else
> -	main_part_return_p = true;
>      }
> -  /* The main part also returns if we split on a fallthru edge
> -     and the split part returns.  */
> -  if (split_part_return_p)
> -    FOR_EACH_EDGE (e, ei, split_point->entry_bb->preds)
> -      {
> -	if (! bitmap_bit_p (split_point->split_bbs, e->src->index)
> -	    && single_succ_p (e->src))
> -	  {
> -	    main_part_return_p = true;
> -	    break;
> -	  }
> -      }
>  
>    /* Add return block to what will become the split function.
>       We do not return; no return block is needed.  */
> @@ -1279,8 +1264,8 @@ split_function (basic_block return_bb, s
>       FIXME: Once we are able to change return type, we should change function
>       to return void instead of just outputting function with undefined return
>       value.  For structures this affects quality of codegen.  */
> -  else if (!split_point->split_part_set_retval
> -           && (retval = find_retval (return_bb)))
> +  else if ((retval = find_retval (return_bb))
> +	   && !split_point->split_part_set_retval)
>      {
>        bool redirected = true;
>        basic_block new_return_bb = create_basic_block (NULL, 0, return_bb);
> @@ -1308,12 +1293,10 @@ split_function (basic_block return_bb, s
>      }
>    /* When we pass around the value, use existing return block.  */
>    else
> -    bitmap_set_bit (split_point->split_bbs, return_bb->index);
> -
> -  /* If the main part doesn't return pretend the return block wasn't
> -     found for all of the following.  */
> -  if (! main_part_return_p)
> -    return_bb = EXIT_BLOCK_PTR_FOR_FN (cfun);
> +    {
> +      bitmap_set_bit (split_point->split_bbs, return_bb->index);
> +      retbnd = find_retbnd (return_bb);
> +    }
>  
>    /* If RETURN_BB has virtual operand PHIs, they must be removed and the
>       virtual operand marked for renaming as we change the CFG in a way that
> @@ -1382,6 +1365,44 @@ split_function (basic_block return_bb, s
>        DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
>      }
>  
> +  /* If return_bb contains any clobbers that refer to SSA_NAMEs
> +     set in the split part, remove them.  Also reset debug stmts that
> +     refer to SSA_NAMEs set in the split part.  */
> +  if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
> +    {
> +      gimple_stmt_iterator gsi = gsi_start_bb (return_bb);
> +      while (!gsi_end_p (gsi))
> +	{
> +	  tree op;
> +	  ssa_op_iter iter;
> +	  gimple *stmt = gsi_stmt (gsi);
> +	  bool remove = false;
> +	  if (gimple_clobber_p (stmt) || is_gimple_debug (stmt))
> +	    FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> +	      {
> +		basic_block bb = gimple_bb (SSA_NAME_DEF_STMT (op));
> +		if (op != retval
> +		    && bb
> +		    && bb != return_bb
> +		    && bitmap_bit_p (split_point->split_bbs, bb->index))
> +		  {
> +		    if (is_gimple_debug (stmt))
> +		      {
> +			gimple_debug_bind_reset_value (stmt);
> +			update_stmt (stmt);
> +		      }
> +		    else
> +		      remove = true;
> +		    break;
> +		  }
> +	      }
> +	  if (remove)
> +	    gsi_remove (&gsi, true);
> +	  else
> +	    gsi_next (&gsi);
> +	}
> +    }
> +
>    /* If the original function is instrumented then it's
>       part is also instrumented.  */
>    if (with_bounds)
> @@ -1499,9 +1520,7 @@ split_function (basic_block return_bb, s
>           return value into and put call just before it.  */
>        if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
>  	{
> -	  real_retval = retval = find_retval (return_bb);
> -	  retbnd = find_retbnd (return_bb);
> -
> +	  real_retval = retval;
>  	  if (real_retval && split_point->split_part_set_retval)
>  	    {
>  	      gphi_iterator psi;
> @@ -1545,6 +1564,28 @@ split_function (basic_block return_bb, s
>  			    break;
>  			  }
>  		      update_stmt (gsi_stmt (bsi));
> +		      /* Also adjust clobbers and debug stmts in return_bb.  */
> +		      for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi);
> +			   gsi_next (&bsi))
> +			{
> +			  gimple *stmt = gsi_stmt (bsi);
> +			  if (gimple_clobber_p (stmt)
> +			      || is_gimple_debug (stmt))
> +			    {
> +			      ssa_op_iter iter;
> +			      use_operand_p use_p;
> +			      bool update = false;
> +			      FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter,
> +							SSA_OP_USE)
> +				if (USE_FROM_PTR (use_p) == real_retval)
> +				  {
> +				    SET_USE (use_p, retval);
> +				    update = true;
> +				  }
> +			      if (update)
> +				update_stmt (stmt);
> +			    }
> +			}
>  		    }
>  
>  		  /* Replace retbnd with new one.  */
> --- gcc/testsuite/g++.dg/ipa/pr68672-1.C.jj	2016-02-11 12:31:27.344497187 +0100
> +++ gcc/testsuite/g++.dg/ipa/pr68672-1.C	2016-02-11 12:31:01.000000000 +0100
> @@ -0,0 +1,20 @@
> +// PR ipa/68672
> +// { dg-do compile }
> +// { dg-options "-O -finline-small-functions -fpartial-inlining --param=partial-inlining-entry-probability=100" }
> +
> +void f2 (void *);
> +void *a;
> +struct C { virtual void m1 (); };
> +struct D { C *m2 () { if (a) __builtin_abort (); } };
> +D f1 ();
> +struct E { int e; ~E () { if (e) f2 (&e); } };
> +E *b;
> +struct I { virtual void m3 (); };
> +
> +void
> +I::m3 ()
> +{
> +  if (a)
> +    f1 ().m2 ()->m1 ();
> +  b->~E ();
> +}
> --- gcc/testsuite/g++.dg/ipa/pr68672-2.C.jj	2016-02-11 12:33:50.744438948 +0100
> +++ gcc/testsuite/g++.dg/ipa/pr68672-2.C	2016-02-11 12:32:50.000000000 +0100
> @@ -0,0 +1,54 @@
> +// PR ipa/68672
> +// { dg-do compile }
> +// { dg-options "-O3 --param=partial-inlining-entry-probability=100 -g" }
> +
> +struct S { ~S () {} };
> +S *a;
> +int *b;
> +void bar ();
> +void baz ();
> +void fn (int *);
> +
> +static int
> +foo ()
> +{
> +  S *c = a;
> +  if (c)
> +    {
> +      bar ();
> +      if (a)
> +	__builtin_abort ();
> +      baz ();
> +    }
> +  int p = *b;
> +  if (p)
> +    {
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +    }
> +  c->~S ();
> +  int q = 2 * p;
> +  int r = 3 * q;
> +  S *d = c;
> +  return p;
> +}
> +
> +void
> +use1 ()
> +{
> +  foo ();
> +}
> +
> +void
> +use2 ()
> +{
> +  foo ();
> +}
> +
> +void
> +use3 ()
> +{
> +  foo ();
> +}
> --- gcc/testsuite/g++.dg/ipa/pr68672-3.C.jj	2016-02-11 12:34:02.374272024 +0100
> +++ gcc/testsuite/g++.dg/ipa/pr68672-3.C	2016-02-11 12:34:22.337985482 +0100
> @@ -0,0 +1,57 @@
> +// PR ipa/68672
> +// { dg-do compile }
> +// { dg-options "-O3 --param=partial-inlining-entry-probability=100 -g" }
> +
> +struct S { ~S () {} };
> +S *a, *e;
> +int *b;
> +void bar ();
> +void baz ();
> +void fn (int *);
> +void fn2 (S *);
> +
> +static int
> +foo ()
> +{
> +  S *c = a;
> +  if (c)
> +    {
> +      bar ();
> +      if (a)
> +	__builtin_abort ();
> +      baz ();
> +    }
> +  int p = *b;
> +  S *f = e;
> +  if (p)
> +    {
> +      fn2 (f);
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +      fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b);
> +    }
> +  f->~S ();
> +  int q = 2 * p;
> +  int r = 3 * q;
> +  S *d = c;
> +  return p;
> +}
> +
> +void
> +use1 ()
> +{
> +  foo ();
> +}
> +
> +void
> +use2 ()
> +{
> +  foo ();
> +}
> +
> +void
> +use3 ()
> +{
> +  foo ();
> +}
> 
> 	Jakub
> 
> 

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]