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 <ja...@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
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
