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)