Re: [PATCH v1] tree-ssa-sink: Improve code sinking pass.

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Thu, May 18, 2023 at 12:44:28PM +0530, Ajit Agarwal wrote:
> This patch improves code sinking pass to sink statements before call to reduce
> register pressure.

An example would be useful :-)

> 	* tree-ssa-sink.cc (statement_sink_location): Modifed to
> 	move statements before calls.

Spello ("modified").  But, you should write in the imperative mood
anyway, so "modify".  But, every change is a modification, so do without
the fluff altogether?  "Move statements before calls."

> 	(block_call_p): New function.
> 	(def_use_same_block): New function.
> 	(select_best_block): Add heuristics to select the best
> 	blocks in the immediate post dominator.

Please don't break lines
early
it makes things
harder to
read.

:-)

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-20.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */

This is the default, you can just leave it out.

> +/* { dg-options "-O2 -fdump-tree-sink -fdump-tree-optimized -fdump-tree-sink-stats" } */

You don't need -fdump-tree-sink without options since you have
-fdump-tree-sink-stats as well.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-21.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */ 
> +/* { dg-options "-O2 -fdump-tree-sink-stats -fdump-tree-sink-stats" } */

You don't need to say it twice either :-)

> +/* Return TRUE if immediate uses of the defs in
> +   USE occur in the same block as USE, FALSE otherwise.  */
> +
> +bool
> +def_use_same_block (gimple *stmt)
> +{

There is no function parameter "use" here?  STMT instead?

> +  use_operand_p use_p;
> +  def_operand_p def_p;

Neither of these is a predicate.  Lose the _p please?

> +	  if (use_p
> +	      && (gimple_bb (USE_STMT (use_p)) == gimple_bb (stmt)))

Please fit this on one line.  And no parens around random things please.

> +/* Return TRUE if the block has only calls, FALSE otherwise. */
> +
> +bool
> +block_call_p (basic_block bb)

> +	   /* We have already seen a call.  */
> +	   if (is_call)
> +	     return false;
> +
> +	   if (is_gimple_call (stmt))
> +	     is_call = true;
> +	   else
> +	     return false;

> +  if (is_call && i == 1)
> +    return true;
> +
> +  return false;

This doesn't do what the function comment says?  It is very important
that function comments say exactly what a function does.  It can perhaps
leave out some details, but it should be correct by and large.

> +		/* Update sinking point as stmt before call if the sinking block
> +		   has only calls. Otherwise update sinking point as the use
> +		   stmt. */

(two spaces after full stop, twice)

> +		if (gsi_stmt (gsi) == use
> +		    && !is_gimple_call (last_stmt)
> +		    && (gimple_code (last_stmt) != GIMPLE_SWITCH)
> +		    && (gimple_code (last_stmt) != GIMPLE_COND)
> +		    && (gimple_code (last_stmt) != GIMPLE_GOTO)
> +		    && (!gimple_vdef (use) || !def_use_same_block (def_stmt)))

Please no unnecessary parens.  At first I didn't notice the last line
here *does* need it!


Segher