phiopt: Avoid -fcompare-debug bug in phiopt [PR94211]

Hi!

Two years ago, I've added support for up to 2 simple preparation statements
in value_replacement, but the
-      && estimate_num_insns (assign, &eni_time_weights)
+      && estimate_num_insns (bb_seq (middle_bb), &eni_time_weights)
change, meant that we compute the cost of all those statements rather than
just the single assign that has been the single supported non-debug
statement in the bb before, doesn't do what I thought would do, gimple_seq
is just gimple * and thus it can't be really overloaded depending on whether
we pass a single gimple * or a whole sequence.  Which means in the last
two years it doesn't count all the statements, but only the first one.
With -g that happens to be a DEBUG_STMT, or it could be e.g. the first
preparation statement which could be much cheaper than the actual assign.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2020-03-19  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/94211
	* tree-ssa-phiopt.c (value_replacement): Use estimate_num_insns_seq
	instead of estimate_num_insns for bb_seq (middle_bb).  Rename
	emtpy_or_with_defined_p variable to empty_or_with_defined_p, adjust
	all uses.

	* gcc.dg/pr94211.c: New test.


	Jakub

On Thu, 19 Mar 2020, Jakub Jelinek wrote:

> Hi!
> 
> Two years ago, I've added support for up to 2 simple preparation statements
> in value_replacement, but the
> -      && estimate_num_insns (assign, &eni_time_weights)
> +      && estimate_num_insns (bb_seq (middle_bb), &eni_time_weights)
> change, meant that we compute the cost of all those statements rather than
> just the single assign that has been the single supported non-debug
> statement in the bb before, doesn't do what I thought would do, gimple_seq
> is just gimple * and thus it can't be really overloaded depending on whether
> we pass a single gimple * or a whole sequence.  Which means in the last
> two years it doesn't count all the statements, but only the first one.
> With -g that happens to be a DEBUG_STMT, or it could be e.g. the first
> preparation statement which could be much cheaper than the actual assign.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?

OK.

I wonder if we should make gimple vs gimple_seq type safe via a
struct gimple_seq { gimple *first }; wrapper type ...

Thanks,
Richard.

> 2020-03-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/94211
> 	* tree-ssa-phiopt.c (value_replacement): Use estimate_num_insns_seq
> 	instead of estimate_num_insns for bb_seq (middle_bb).  Rename
> 	emtpy_or_with_defined_p variable to empty_or_with_defined_p, adjust
> 	all uses.
> 
> 	* gcc.dg/pr94211.c: New test.
> 
> --- gcc/tree-ssa-phiopt.c.jj	2020-03-16 23:49:29.853404202 +0100
> +++ gcc/tree-ssa-phiopt.c	2020-03-18 19:42:22.583225152 +0100
> @@ -1056,7 +1056,7 @@ value_replacement (basic_block cond_bb,
>    gimple *cond;
>    edge true_edge, false_edge;
>    enum tree_code code;
> -  bool emtpy_or_with_defined_p = true;
> +  bool empty_or_with_defined_p = true;
>  
>    /* If the type says honor signed zeros we cannot do this
>       optimization.  */
> @@ -1075,7 +1075,7 @@ value_replacement (basic_block cond_bb,
>  	{
>  	  if (gimple_code (stmt) != GIMPLE_PREDICT
>  	      && gimple_code (stmt) != GIMPLE_NOP)
> -	    emtpy_or_with_defined_p = false;
> +	    empty_or_with_defined_p = false;
>  	  continue;
>  	}
>        /* Now try to adjust arg0 or arg1 according to the computation
> @@ -1085,7 +1085,7 @@ value_replacement (basic_block cond_bb,
>  	     && jump_function_from_stmt (&arg0, stmt))
>  	    || (lhs == arg1
>  		&& jump_function_from_stmt (&arg1, stmt)))
> -	emtpy_or_with_defined_p = false;
> +	empty_or_with_defined_p = false;
>      }
>  
>    cond = last_stmt (cond_bb);
> @@ -1137,7 +1137,7 @@ value_replacement (basic_block cond_bb,
>        /* If the middle basic block was empty or is defining the
>  	 PHI arguments and this is a single phi where the args are different
>  	 for the edges e0 and e1 then we can remove the middle basic block. */
> -      if (emtpy_or_with_defined_p
> +      if (empty_or_with_defined_p
>  	  && single_non_singleton_phi_for_edges (phi_nodes (gimple_bb (phi)),
>  						 e0, e1) == phi)
>  	{
> @@ -1255,7 +1255,7 @@ value_replacement (basic_block cond_bb,
>        && profile_status_for_fn (cfun) != PROFILE_ABSENT
>        && EDGE_PRED (middle_bb, 0)->probability < profile_probability::even ()
>        /* If assign is cheap, there is no point avoiding it.  */
> -      && estimate_num_insns (bb_seq (middle_bb), &eni_time_weights)
> +      && estimate_num_insns_seq (bb_seq (middle_bb), &eni_time_weights)
>  	 >= 3 * estimate_num_insns (cond, &eni_time_weights))
>      return 0;
>  
> --- gcc/testsuite/gcc.dg/pr94211.c.jj	2020-03-18 16:30:34.562427467 +0100
> +++ gcc/testsuite/gcc.dg/pr94211.c	2020-03-18 16:30:19.998640206 +0100
> @@ -0,0 +1,12 @@
> +/* PR tree-optimization/94211 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fcompare-debug" } */
> +
> +long
> +foo (long a, long b)
> +{
> +  if (__builtin_expect (b == 1, 1))
> +    return a;
> +  int e = a + 1;
> +  return a / b;
> +}
> 
> 	Jakub
> 
>