* [COMMITTED] Kill second order relations in the path solver. @ 2021-10-27 18:13 Aldy Hernandez 2021-10-27 18:13 ` [COMMITTED] Reorder relation calculating code " Aldy Hernandez ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Aldy Hernandez @ 2021-10-27 18:13 UTC (permalink / raw) To: GCC patches My upcoming work replacing the VRP threaders with a fully resolving backward threader has tripped over various corner cases in the path sensitive relation oracle. This patch kills second order relations when we kill a relation. Tested on x86-64 and ppc64le Linux. Co-authored-by: Andrew MacLeod <amacleod@redhat.com> gcc/ChangeLog: * value-relation.cc (path_oracle::killing_def): Kill second order relations. --- gcc/value-relation.cc | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/gcc/value-relation.cc b/gcc/value-relation.cc index 2acf375ca9a..0ad4f7a9495 100644 --- a/gcc/value-relation.cc +++ b/gcc/value-relation.cc @@ -1297,8 +1297,9 @@ path_oracle::killing_def (tree ssa) fprintf (dump_file, "\n"); } + unsigned v = SSA_NAME_VERSION (ssa); bitmap b = BITMAP_ALLOC (&m_bitmaps); - bitmap_set_bit (b, SSA_NAME_VERSION (ssa)); + bitmap_set_bit (b, v); equiv_chain *ptr = (equiv_chain *) obstack_alloc (&m_chain_obstack, sizeof (equiv_chain)); ptr->m_names = b; @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) ptr->m_next = m_equiv.m_next; m_equiv.m_next = ptr; bitmap_ior_into (m_equiv.m_names, b); + + // Walk the relation list an remove SSA from any relations. + if (!bitmap_bit_p (m_relations.m_names, v)) + return; + + bitmap_clear_bit (m_relations.m_names, v); + relation_chain **prev = &(m_relations.m_head); + relation_chain *next = NULL; + for (relation_chain *ptr = m_relations.m_head; ptr; ptr = next) + { + gcc_checking_assert (*prev == ptr); + next = ptr->m_next; + if (SSA_NAME_VERSION (ptr->op1 ()) == v + || SSA_NAME_VERSION (ptr->op2 ()) == v) + *prev = ptr->m_next; + else + prev = &(ptr->m_next); + } } // Register relation K between SSA1 and SSA2, resolving unknowns by -- 2.31.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [COMMITTED] Reorder relation calculating code in the path solver. 2021-10-27 18:13 [COMMITTED] Kill second order relations in the path solver Aldy Hernandez @ 2021-10-27 18:13 ` Aldy Hernandez 2021-10-27 18:13 ` [COMMITTED] Kill known equivalences before a new assignment " Aldy Hernandez 2021-10-27 23:55 ` [COMMITTED] Kill second order relations " Bernhard Reutner-Fischer 2 siblings, 0 replies; 11+ messages in thread From: Aldy Hernandez @ 2021-10-27 18:13 UTC (permalink / raw) To: GCC patches Enabling the fully resolving threader triggers various relation ordering issues that have previously been dormant because the VRP hybrid threader (forward threader based) never gives us long enough paths for this to matter. The new threader spares no punches in finding non-obvious paths, so getting the relations right is paramount. This patch fixes a couple oversights that have gone undetected. First, some background. There are 3 types of relations along a path: a) Relations inherent in a PHI. b) Relations as a side-effect of evaluating a statement. c) Outgoing relations between blocks in a path. We must calculate these in their proper order, otherwise we can run into ordering issues. The current ordering is wrong, as we precalculate PHIs for _all_ blocks before anything else, and then proceed to register the relations throughout the path. Also, we fail to realize that a PHI whose argument is also defined in the PHIs block cannot be registered as an equivalence without causing more ordering issues. This patch fixes all the problems described above. With it we get a handful more net threads, but most importantly, we disallow some threads that were wrong. Tested on x86-64 and ppc64le Linux on the usual regstrap, plus by comparing the different thread counts before and after this patch. gcc/ChangeLog: * gimple-range-fold.cc (fold_using_range::range_of_range_op): Dump operands as well as relation. * gimple-range-path.cc (path_range_query::compute_ranges_in_block): Compute PHI relations first. Compute outgoing relations at the end. (path_range_query::compute_ranges): Remove call to compute_relations. (path_range_query::compute_relations): Remove. (path_range_query::maybe_register_phi_relation): New. (path_range_query::compute_phi_relations): Abstract out registering one PHI relation to... (path_range_query::compute_outgoing_relations): ...here. * gimple-range-path.h (class path_range_query): Remove compute_relations. Add maybe_register_phi_relation. --- gcc/gimple-range-fold.cc | 2 + gcc/gimple-range-path.cc | 107 ++++++++++++++++++++------------------- gcc/gimple-range-path.h | 3 +- 3 files changed, 58 insertions(+), 54 deletions(-) diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc index ed2fbe121cf..2fab904e6b0 100644 --- a/gcc/gimple-range-fold.cc +++ b/gcc/gimple-range-fold.cc @@ -620,7 +620,9 @@ fold_using_range::range_of_range_op (irange &r, gimple *s, fur_source &src) if (dump_file && (dump_flags & TDF_DETAILS) && rel != VREL_NONE) { fprintf (dump_file, " folding with relation "); + print_generic_expr (dump_file, op1, TDF_SLIM); print_relation (dump_file, rel); + print_generic_expr (dump_file, op2, TDF_SLIM); fputc ('\n', dump_file); } // Fold range, and register any dependency if available. diff --git a/gcc/gimple-range-path.cc b/gcc/gimple-range-path.cc index 557338993ae..2f570a13e05 100644 --- a/gcc/gimple-range-path.cc +++ b/gcc/gimple-range-path.cc @@ -316,6 +316,9 @@ path_range_query::compute_ranges_in_block (basic_block bb) int_range_max r, cached_range; unsigned i; + if (m_resolve && !at_entry ()) + compute_phi_relations (bb, prev_bb ()); + // Force recalculation of any names in the cache that are defined in // this block. This can happen on interdependent SSA/phis in loops. EXECUTE_IF_SET_IN_BITMAP (m_imports, 0, i, bi) @@ -341,7 +344,8 @@ path_range_query::compute_ranges_in_block (basic_block bb) return; // Solve imports that are exported to the next block. - edge e = find_edge (bb, next_bb ()); + basic_block next = next_bb (); + edge e = find_edge (bb, next); EXECUTE_IF_SET_IN_BITMAP (m_imports, 0, i, bi) { tree name = ssa_name (i); @@ -369,6 +373,9 @@ path_range_query::compute_ranges_in_block (basic_block bb) } } } + + if (m_resolve) + compute_outgoing_relations (bb, next); } // Adjust all pointer imports in BB with non-null information. @@ -485,7 +492,6 @@ path_range_query::compute_ranges (const vec<basic_block> &path, { add_copies_to_imports (); get_path_oracle ()->reset_path (); - compute_relations (path); } if (DEBUG_SOLVER) @@ -527,7 +533,12 @@ path_range_query::compute_ranges (const vec<basic_block> &path, } if (DEBUG_SOLVER) - dump (dump_file); + { + fprintf (dump_file, "\npath_oracle:\n"); + get_path_oracle ()->dump (dump_file); + fprintf (dump_file, "\n"); + dump (dump_file); + } } // A folding aid used to register and query relations along a path. @@ -624,49 +635,23 @@ path_range_query::range_of_stmt (irange &r, gimple *stmt, tree) return true; } -// Compute relations on a path. This involves two parts: relations -// along the conditionals joining a path, and relations determined by -// examining PHIs. - void -path_range_query::compute_relations (const vec<basic_block> &path) +path_range_query::maybe_register_phi_relation (gphi *phi, tree arg) { - if (!dom_info_available_p (CDI_DOMINATORS)) - return; + basic_block bb = gimple_bb (phi); + tree result = gimple_phi_result (phi); + gimple *def = SSA_NAME_DEF_STMT (arg); - jt_fur_source src (NULL, this, &m_ranger.gori (), path); - basic_block prev = NULL; - for (unsigned i = path.length (); i > 0; --i) - { - basic_block bb = path[i - 1]; - gimple *stmt = last_stmt (bb); + // Avoid recording the equivalence if the arg is defined in this + // block, as that would create an ordering problem. + if (def && gimple_bb (def) == bb) + return; - compute_phi_relations (bb, prev); + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, " from bb%d:", bb->index); - // Compute relations in outgoing edges along the path. Skip the - // final conditional which we don't know yet. - if (i > 1 - && stmt - && gimple_code (stmt) == GIMPLE_COND - && irange::supports_type_p (TREE_TYPE (gimple_cond_lhs (stmt)))) - { - basic_block next = path[i - 2]; - int_range<2> r; - gcond *cond = as_a<gcond *> (stmt); - edge e0 = EDGE_SUCC (bb, 0); - edge e1 = EDGE_SUCC (bb, 1); - - if (e0->dest == next) - gcond_edge_range (r, e0); - else if (e1->dest == next) - gcond_edge_range (r, e1); - else - gcc_unreachable (); - - src.register_outgoing_edges (cond, r, e0, e1); - } - prev = bb; - } + get_path_oracle ()->killing_def (result); + m_oracle->register_relation (entry_bb (), EQ_EXPR, arg, result); } // Compute relations for each PHI in BB. For example: @@ -681,15 +666,12 @@ path_range_query::compute_phi_relations (basic_block bb, basic_block prev) if (prev == NULL) return; - basic_block entry = entry_bb (); edge e_in = find_edge (prev, bb); - gcc_checking_assert (e_in); for (gphi_iterator iter = gsi_start_phis (bb); !gsi_end_p (iter); gsi_next (&iter)) { gphi *phi = iter.phi (); - tree result = gimple_phi_result (phi); unsigned nargs = gimple_phi_num_args (phi); for (size_t i = 0; i < nargs; ++i) @@ -698,17 +680,36 @@ path_range_query::compute_phi_relations (basic_block bb, basic_block prev) tree arg = gimple_phi_arg_def (phi, i); if (gimple_range_ssa_p (arg)) - { - if (dump_file && (dump_flags & TDF_DETAILS)) - fprintf (dump_file, " from bb%d:", bb->index); + maybe_register_phi_relation (phi, arg); + break; + } + } +} - // Throw away any previous relation. - get_path_oracle ()->killing_def (result); +// Compute outgoing relations from BB to NEXT. - m_oracle->register_relation (entry, EQ_EXPR, arg, result); - } +void +path_range_query::compute_outgoing_relations (basic_block bb, basic_block next) +{ + gimple *stmt = last_stmt (bb); - break; - } + if (stmt + && gimple_code (stmt) == GIMPLE_COND + && irange::supports_type_p (TREE_TYPE (gimple_cond_lhs (stmt)))) + { + int_range<2> r; + gcond *cond = as_a<gcond *> (stmt); + edge e0 = EDGE_SUCC (bb, 0); + edge e1 = EDGE_SUCC (bb, 1); + + if (e0->dest == next) + gcond_edge_range (r, e0); + else if (e1->dest == next) + gcond_edge_range (r, e1); + else + gcc_unreachable (); + + jt_fur_source src (NULL, this, &m_ranger.gori (), *m_path); + src.register_outgoing_edges (cond, r, e0, e1); } } diff --git a/gcc/gimple-range-path.h b/gcc/gimple-range-path.h index 5f4e73a5949..541613956e1 100644 --- a/gcc/gimple-range-path.h +++ b/gcc/gimple-range-path.h @@ -57,8 +57,9 @@ private: void compute_ranges_in_block (basic_block bb); void adjust_for_non_null_uses (basic_block bb); void ssa_range_in_phi (irange &r, gphi *phi); - void compute_relations (const vec<basic_block> &); + void compute_outgoing_relations (basic_block bb, basic_block next); void compute_phi_relations (basic_block bb, basic_block prev); + void maybe_register_phi_relation (gphi *, tree arg); void add_copies_to_imports (); bool add_to_imports (tree name, bitmap imports); -- 2.31.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [COMMITTED] Kill known equivalences before a new assignment in the path solver. 2021-10-27 18:13 [COMMITTED] Kill second order relations in the path solver Aldy Hernandez 2021-10-27 18:13 ` [COMMITTED] Reorder relation calculating code " Aldy Hernandez @ 2021-10-27 18:13 ` Aldy Hernandez 2021-10-27 23:55 ` [COMMITTED] Kill second order relations " Bernhard Reutner-Fischer 2 siblings, 0 replies; 11+ messages in thread From: Aldy Hernandez @ 2021-10-27 18:13 UTC (permalink / raw) To: GCC patches Every time we have a killing statement, we must also kill the relations seen so far. This is similar to what we did for the equivs inherent in PHIs along a path. Tested on x86-64 and ppc64le Linux. gcc/ChangeLog: * gimple-range-path.cc (path_range_query::range_defined_in_block): Call killing_def. --- gcc/gimple-range-path.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/gcc/gimple-range-path.cc b/gcc/gimple-range-path.cc index 2f570a13e05..d8c2a9b6a86 100644 --- a/gcc/gimple-range-path.cc +++ b/gcc/gimple-range-path.cc @@ -288,8 +288,14 @@ path_range_query::range_defined_in_block (irange &r, tree name, basic_block bb) if (gimple_code (def_stmt) == GIMPLE_PHI) ssa_range_in_phi (r, as_a<gphi *> (def_stmt)); - else if (!range_of_stmt (r, def_stmt, name)) - r.set_varying (TREE_TYPE (name)); + else + { + if (name) + get_path_oracle ()->killing_def (name); + + if (!range_of_stmt (r, def_stmt, name)) + r.set_varying (TREE_TYPE (name)); + } if (bb) m_non_null.adjust_range (r, name, bb); -- 2.31.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [COMMITTED] Kill second order relations in the path solver. 2021-10-27 18:13 [COMMITTED] Kill second order relations in the path solver Aldy Hernandez 2021-10-27 18:13 ` [COMMITTED] Reorder relation calculating code " Aldy Hernandez 2021-10-27 18:13 ` [COMMITTED] Kill known equivalences before a new assignment " Aldy Hernandez @ 2021-10-27 23:55 ` Bernhard Reutner-Fischer 2021-11-01 14:10 ` redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] Bernhard Reutner-Fischer 2 siblings, 1 reply; 11+ messages in thread From: Bernhard Reutner-Fischer @ 2021-10-27 23:55 UTC (permalink / raw) To: Aldy Hernandez; +Cc: rep.dot.nop, Andrew MacLeod, GCC patches On Wed, 27 Oct 2021 20:13:21 +0200 Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: [would have to think about this some more but it's late here. Nits:] > diff --git a/gcc/value-relation.cc b/gcc/value-relation.cc > index 2acf375ca9a..0ad4f7a9495 100644 > --- a/gcc/value-relation.cc > +++ b/gcc/value-relation.cc > @@ -1297,8 +1297,9 @@ path_oracle::killing_def (tree ssa) > fprintf (dump_file, "\n"); > } > > + unsigned v = SSA_NAME_VERSION (ssa); > bitmap b = BITMAP_ALLOC (&m_bitmaps); > - bitmap_set_bit (b, SSA_NAME_VERSION (ssa)); > + bitmap_set_bit (b, v); > equiv_chain *ptr = (equiv_chain *) obstack_alloc (&m_chain_obstack, > sizeof (equiv_chain)); > ptr->m_names = b; > @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) > ptr->m_next = m_equiv.m_next; > m_equiv.m_next = ptr; > bitmap_ior_into (m_equiv.m_names, b); > + > + // Walk the relation list an remove SSA from any relations. s/an /and / > + if (!bitmap_bit_p (m_relations.m_names, v)) > + return; > + > + bitmap_clear_bit (m_relations.m_names, v); IIRC bitmap_clear_bit returns true if the bit was set, false otherwise, so should be used as if(!bitmap_clear_bit) above. I would not be surprised if this generates better code as we probably do not grok to optimize the !bit_p else clear_bit combo. Shame (?). > + relation_chain **prev = &(m_relations.m_head); s/[()]// thanks, > + relation_chain *next = NULL; > + for (relation_chain *ptr = m_relations.m_head; ptr; ptr = next) > + { > + gcc_checking_assert (*prev == ptr); > + next = ptr->m_next; > + if (SSA_NAME_VERSION (ptr->op1 ()) == v > + || SSA_NAME_VERSION (ptr->op2 ()) == v) > + *prev = ptr->m_next; > + else > + prev = &(ptr->m_next); > + } > } > > // Register relation K between SSA1 and SSA2, resolving unknowns by ^ permalink raw reply [flat|nested] 11+ messages in thread
* redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] 2021-10-27 23:55 ` [COMMITTED] Kill second order relations " Bernhard Reutner-Fischer @ 2021-11-01 14:10 ` Bernhard Reutner-Fischer 2021-11-01 14:21 ` Aldy Hernandez 0 siblings, 1 reply; 11+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-01 14:10 UTC (permalink / raw) To: Aldy Hernandez; +Cc: rep.dot.nop, Andrew MacLeod, GCC patches On Thu, 28 Oct 2021 01:55:30 +0200 Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > On Wed, 27 Oct 2021 20:13:21 +0200 > Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) > > ptr->m_next = m_equiv.m_next; > > m_equiv.m_next = ptr; > > bitmap_ior_into (m_equiv.m_names, b); > > + > > + // Walk the relation list an remove SSA from any relations. > > s/an /and / > > > + if (!bitmap_bit_p (m_relations.m_names, v)) > > + return; > > + > > + bitmap_clear_bit (m_relations.m_names, v); > > IIRC bitmap_clear_bit returns true if the bit was set, false otherwise, > so should be used as if(!bitmap_clear_bit) above. > > + relation_chain **prev = &(m_relations.m_head); > > s/[()]// > thanks, There seems to be two other spots where a redundant bitmap_bit_p checks if we want to bitmap_clear_bit. In dse and ira. Like: $ cat ~/coccinelle/gcc_bitmap_bit_p-0.cocci ; echo EOF // replace redundant bitmap_bit_p() bitmap_clear_bit with the latter @ rule1 @ identifier fn; expression bitmap, bit; @@ fn(...) { <... ( -if (bitmap_bit_p (bitmap, bit)) +if (bitmap_clear_bit (bitmap, bit)) { ... - bitmap_clear_bit (bitmap, bit); ... } | -if (bitmap_bit_p (bitmap, bit)) +if (bitmap_clear_bit (bitmap, bit)) { ... } ... -bitmap_clear_bit (bitmap, bit); ) ...> } EOF $ find gcc/ -type f -a \( -name "*.c" -o -name "*.cc" \) -a \( ! -path "gcc/testsuite/*" -a ! -path "gcc/contrib/*" \) -exec spatch -sp_file ~/coccinelle/gcc_bitmap_bit_p-0.cocci --show-diff {} \; diff = --- gcc/dse.c +++ /tmp/cocci-output-1104419-443759-dse.c @@ -3238,9 +3238,8 @@ mark_reachable_blocks (sbitmap unreachab edge e; edge_iterator ei; - if (bitmap_bit_p (unreachable_blocks, bb->index)) + if (bitmap_clear_bit(unreachable_blocks, bb->index)) { - bitmap_clear_bit (unreachable_blocks, bb->index); FOR_EACH_EDGE (e, ei, bb->preds) { mark_reachable_blocks (unreachable_blocks, e->src); diff = --- gcc/ira.c +++ /tmp/cocci-output-1104678-d8679a-ira.c @@ -2944,17 +2944,15 @@ mark_elimination (int from, int to) FOR_EACH_BB_FN (bb, cfun) { r = DF_LR_IN (bb); - if (bitmap_bit_p (r, from)) + if (bitmap_clear_bit(r, from)) { - bitmap_clear_bit (r, from); bitmap_set_bit (r, to); } if (! df_live) continue; r = DF_LIVE_IN (bb); - if (bitmap_bit_p (r, from)) + if (bitmap_clear_bit(r, from)) { - bitmap_clear_bit (r, from); bitmap_set_bit (r, to); } } # in ira.c one would have to fixup the curly braces manually PS: coccinelle seems to ruin the spaces before braces in the '+' even though i have written them correctly according to GNU style.. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] 2021-11-01 14:10 ` redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] Bernhard Reutner-Fischer @ 2021-11-01 14:21 ` Aldy Hernandez 2021-11-01 21:02 ` Bernhard Reutner-Fischer 0 siblings, 1 reply; 11+ messages in thread From: Aldy Hernandez @ 2021-11-01 14:21 UTC (permalink / raw) To: Bernhard Reutner-Fischer; +Cc: Andrew MacLeod, GCC patches I'm not convinced this makes the code clearer to read, especially if it's not on a critical path. But if you feel strongly, please submit a patch ;-). Aldy On Mon, Nov 1, 2021 at 3:10 PM Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > > On Thu, 28 Oct 2021 01:55:30 +0200 > Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > > > On Wed, 27 Oct 2021 20:13:21 +0200 > > Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) > > > ptr->m_next = m_equiv.m_next; > > > m_equiv.m_next = ptr; > > > bitmap_ior_into (m_equiv.m_names, b); > > > + > > > + // Walk the relation list an remove SSA from any relations. > > > > s/an /and / > > > > > + if (!bitmap_bit_p (m_relations.m_names, v)) > > > + return; > > > + > > > + bitmap_clear_bit (m_relations.m_names, v); > > > > IIRC bitmap_clear_bit returns true if the bit was set, false otherwise, > > so should be used as if(!bitmap_clear_bit) above. > > > > + relation_chain **prev = &(m_relations.m_head); > > > > s/[()]// > > thanks, > > There seems to be two other spots where a redundant bitmap_bit_p checks > if we want to bitmap_clear_bit. In dse and ira. > Like: > $ cat ~/coccinelle/gcc_bitmap_bit_p-0.cocci ; echo EOF > // replace redundant bitmap_bit_p() bitmap_clear_bit with the latter > @ rule1 @ > identifier fn; > expression bitmap, bit; > @@ > > fn(...) { > <... > ( > -if (bitmap_bit_p (bitmap, bit)) > +if (bitmap_clear_bit (bitmap, bit)) > { > ... > - bitmap_clear_bit (bitmap, bit); > ... > } > | > -if (bitmap_bit_p (bitmap, bit)) > +if (bitmap_clear_bit (bitmap, bit)) > { > ... > } > ... > -bitmap_clear_bit (bitmap, bit); > ) > ...> > } > EOF > $ find gcc/ -type f -a \( -name "*.c" -o -name "*.cc" \) -a \( ! -path "gcc/testsuite/*" -a ! -path "gcc/contrib/*" \) -exec spatch -sp_file ~/coccinelle/gcc_bitmap_bit_p-0.cocci --show-diff {} \; > diff = > --- gcc/dse.c > +++ /tmp/cocci-output-1104419-443759-dse.c > @@ -3238,9 +3238,8 @@ mark_reachable_blocks (sbitmap unreachab > edge e; > edge_iterator ei; > > - if (bitmap_bit_p (unreachable_blocks, bb->index)) > + if (bitmap_clear_bit(unreachable_blocks, bb->index)) > { > - bitmap_clear_bit (unreachable_blocks, bb->index); > FOR_EACH_EDGE (e, ei, bb->preds) > { > mark_reachable_blocks (unreachable_blocks, e->src); > diff = > --- gcc/ira.c > +++ /tmp/cocci-output-1104678-d8679a-ira.c > @@ -2944,17 +2944,15 @@ mark_elimination (int from, int to) > FOR_EACH_BB_FN (bb, cfun) > { > r = DF_LR_IN (bb); > - if (bitmap_bit_p (r, from)) > + if (bitmap_clear_bit(r, from)) > { > - bitmap_clear_bit (r, from); > bitmap_set_bit (r, to); > } > if (! df_live) > continue; > r = DF_LIVE_IN (bb); > - if (bitmap_bit_p (r, from)) > + if (bitmap_clear_bit(r, from)) > { > - bitmap_clear_bit (r, from); > bitmap_set_bit (r, to); > } > } > # in ira.c one would have to fixup the curly braces manually > PS: coccinelle seems to ruin the spaces before braces in the '+' even > though i have written them correctly according to GNU style.. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] 2021-11-01 14:21 ` Aldy Hernandez @ 2021-11-01 21:02 ` Bernhard Reutner-Fischer 2021-11-02 13:43 ` Richard Biener 0 siblings, 1 reply; 11+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-01 21:02 UTC (permalink / raw) To: Aldy Hernandez; +Cc: rep.dot.nop, Andrew MacLeod, GCC patches On Mon, 1 Nov 2021 15:21:03 +0100 Aldy Hernandez <aldyh@redhat.com> wrote: > I'm not convinced this makes the code clearer to read, especially if > it's not on a critical path. But if you feel strongly, please submit > a patch ;-). No i don't feel strongly about it. Compiling e.g. -O2 ira.o # Overhead Samples Command Shared Object Symbol # ........ ............ ....... ............. ......................... # 100.00% 4197 cc1plus cc1plus [.] mark_reachable_blocks 100.00% 22000 cc1plus cc1plus [.] path_oracle::killing_def and the mark_elimination is reload. So it's not just a handful of calls saved but some. And it would be smaller code as it saves a call. Well maybe another day. thanks, > > Aldy > > On Mon, Nov 1, 2021 at 3:10 PM Bernhard Reutner-Fischer > <rep.dot.nop@gmail.com> wrote: > > > > On Thu, 28 Oct 2021 01:55:30 +0200 > > Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > > > > > On Wed, 27 Oct 2021 20:13:21 +0200 > > > Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > > @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) > > > > ptr->m_next = m_equiv.m_next; > > > > m_equiv.m_next = ptr; > > > > bitmap_ior_into (m_equiv.m_names, b); > > > > + > > > > + // Walk the relation list an remove SSA from any relations. > > > > > > s/an /and / > > > > > > > + if (!bitmap_bit_p (m_relations.m_names, v)) > > > > + return; > > > > + > > > > + bitmap_clear_bit (m_relations.m_names, v); > > > > > > IIRC bitmap_clear_bit returns true if the bit was set, false otherwise, > > > so should be used as if(!bitmap_clear_bit) above. > > > > > > + relation_chain **prev = &(m_relations.m_head); > > > > > > s/[()]// > > > thanks, > > > > There seems to be two other spots where a redundant bitmap_bit_p checks > > if we want to bitmap_clear_bit. In dse and ira. > > Like: > > $ cat ~/coccinelle/gcc_bitmap_bit_p-0.cocci ; echo EOF > > // replace redundant bitmap_bit_p() bitmap_clear_bit with the latter > > @ rule1 @ > > identifier fn; > > expression bitmap, bit; > > @@ > > > > fn(...) { > > <... > > ( > > -if (bitmap_bit_p (bitmap, bit)) > > +if (bitmap_clear_bit (bitmap, bit)) > > { > > ... > > - bitmap_clear_bit (bitmap, bit); > > ... > > } > > | > > -if (bitmap_bit_p (bitmap, bit)) > > +if (bitmap_clear_bit (bitmap, bit)) > > { > > ... > > } > > ... > > -bitmap_clear_bit (bitmap, bit); > > ) > > ...> > > } > > EOF > > $ find gcc/ -type f -a \( -name "*.c" -o -name "*.cc" \) -a \( ! -path "gcc/testsuite/*" -a ! -path "gcc/contrib/*" \) -exec spatch -sp_file ~/coccinelle/gcc_bitmap_bit_p-0.cocci --show-diff {} \; > > diff = > > --- gcc/dse.c > > +++ /tmp/cocci-output-1104419-443759-dse.c > > @@ -3238,9 +3238,8 @@ mark_reachable_blocks (sbitmap unreachab > > edge e; > > edge_iterator ei; > > > > - if (bitmap_bit_p (unreachable_blocks, bb->index)) > > + if (bitmap_clear_bit(unreachable_blocks, bb->index)) > > { > > - bitmap_clear_bit (unreachable_blocks, bb->index); > > FOR_EACH_EDGE (e, ei, bb->preds) > > { > > mark_reachable_blocks (unreachable_blocks, e->src); > > diff = > > --- gcc/ira.c > > +++ /tmp/cocci-output-1104678-d8679a-ira.c > > @@ -2944,17 +2944,15 @@ mark_elimination (int from, int to) > > FOR_EACH_BB_FN (bb, cfun) > > { > > r = DF_LR_IN (bb); > > - if (bitmap_bit_p (r, from)) > > + if (bitmap_clear_bit(r, from)) > > { > > - bitmap_clear_bit (r, from); > > bitmap_set_bit (r, to); > > } > > if (! df_live) > > continue; > > r = DF_LIVE_IN (bb); > > - if (bitmap_bit_p (r, from)) > > + if (bitmap_clear_bit(r, from)) > > { > > - bitmap_clear_bit (r, from); > > bitmap_set_bit (r, to); > > } > > } > > # in ira.c one would have to fixup the curly braces manually > > PS: coccinelle seems to ruin the spaces before braces in the '+' even > > though i have written them correctly according to GNU style.. > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] 2021-11-01 21:02 ` Bernhard Reutner-Fischer @ 2021-11-02 13:43 ` Richard Biener 2021-11-02 18:01 ` Richard Sandiford 2021-11-02 21:00 ` Bernhard Reutner-Fischer 0 siblings, 2 replies; 11+ messages in thread From: Richard Biener @ 2021-11-02 13:43 UTC (permalink / raw) To: Bernhard Reutner-Fischer; +Cc: Aldy Hernandez, GCC patches On Mon, Nov 1, 2021 at 10:02 PM Bernhard Reutner-Fischer via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Mon, 1 Nov 2021 15:21:03 +0100 > Aldy Hernandez <aldyh@redhat.com> wrote: > > > I'm not convinced this makes the code clearer to read, especially if > > it's not on a critical path. But if you feel strongly, please submit > > a patch ;-). > > No i don't feel strongly about it. > Compiling e.g. -O2 ira.o > # Overhead Samples Command Shared Object Symbol > # ........ ............ ....... ............. ......................... > # > 100.00% 4197 cc1plus cc1plus [.] mark_reachable_blocks > 100.00% 22000 cc1plus cc1plus [.] path_oracle::killing_def > and the mark_elimination is reload. > So it's not just a handful of calls saved but some. And it would be > smaller code as it saves a call. Well maybe another day. Note that single bit set/clear are already implemented as test && set/clear. Note that unfortunately the sbitmap bitmap_set/clear_bit overloads do not return the previous state of the bit. Maybe providing bitmap_test_and_set_bit () and bitmap_test_and_clear_bit () would be better (but note we currently return true when the bit changed, not when it was set). Richard. > thanks, > > > > Aldy > > > > On Mon, Nov 1, 2021 at 3:10 PM Bernhard Reutner-Fischer > > <rep.dot.nop@gmail.com> wrote: > > > > > > On Thu, 28 Oct 2021 01:55:30 +0200 > > > Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > > > > > > > On Wed, 27 Oct 2021 20:13:21 +0200 > > > > Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > > > > @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) > > > > > ptr->m_next = m_equiv.m_next; > > > > > m_equiv.m_next = ptr; > > > > > bitmap_ior_into (m_equiv.m_names, b); > > > > > + > > > > > + // Walk the relation list an remove SSA from any relations. > > > > > > > > s/an /and / > > > > > > > > > + if (!bitmap_bit_p (m_relations.m_names, v)) > > > > > + return; > > > > > + > > > > > + bitmap_clear_bit (m_relations.m_names, v); > > > > > > > > IIRC bitmap_clear_bit returns true if the bit was set, false otherwise, > > > > so should be used as if(!bitmap_clear_bit) above. > > > > > > > > + relation_chain **prev = &(m_relations.m_head); > > > > > > > > s/[()]// > > > > thanks, > > > > > > There seems to be two other spots where a redundant bitmap_bit_p checks > > > if we want to bitmap_clear_bit. In dse and ira. > > > Like: > > > $ cat ~/coccinelle/gcc_bitmap_bit_p-0.cocci ; echo EOF > > > // replace redundant bitmap_bit_p() bitmap_clear_bit with the latter > > > @ rule1 @ > > > identifier fn; > > > expression bitmap, bit; > > > @@ > > > > > > fn(...) { > > > <... > > > ( > > > -if (bitmap_bit_p (bitmap, bit)) > > > +if (bitmap_clear_bit (bitmap, bit)) > > > { > > > ... > > > - bitmap_clear_bit (bitmap, bit); > > > ... > > > } > > > | > > > -if (bitmap_bit_p (bitmap, bit)) > > > +if (bitmap_clear_bit (bitmap, bit)) > > > { > > > ... > > > } > > > ... > > > -bitmap_clear_bit (bitmap, bit); > > > ) > > > ...> > > > } > > > EOF > > > $ find gcc/ -type f -a \( -name "*.c" -o -name "*.cc" \) -a \( ! -path "gcc/testsuite/*" -a ! -path "gcc/contrib/*" \) -exec spatch -sp_file ~/coccinelle/gcc_bitmap_bit_p-0.cocci --show-diff {} \; > > > diff = > > > --- gcc/dse.c > > > +++ /tmp/cocci-output-1104419-443759-dse.c > > > @@ -3238,9 +3238,8 @@ mark_reachable_blocks (sbitmap unreachab > > > edge e; > > > edge_iterator ei; > > > > > > - if (bitmap_bit_p (unreachable_blocks, bb->index)) > > > + if (bitmap_clear_bit(unreachable_blocks, bb->index)) > > > { > > > - bitmap_clear_bit (unreachable_blocks, bb->index); > > > FOR_EACH_EDGE (e, ei, bb->preds) > > > { > > > mark_reachable_blocks (unreachable_blocks, e->src); > > > diff = > > > --- gcc/ira.c > > > +++ /tmp/cocci-output-1104678-d8679a-ira.c > > > @@ -2944,17 +2944,15 @@ mark_elimination (int from, int to) > > > FOR_EACH_BB_FN (bb, cfun) > > > { > > > r = DF_LR_IN (bb); > > > - if (bitmap_bit_p (r, from)) > > > + if (bitmap_clear_bit(r, from)) > > > { > > > - bitmap_clear_bit (r, from); > > > bitmap_set_bit (r, to); > > > } > > > if (! df_live) > > > continue; > > > r = DF_LIVE_IN (bb); > > > - if (bitmap_bit_p (r, from)) > > > + if (bitmap_clear_bit(r, from)) > > > { > > > - bitmap_clear_bit (r, from); > > > bitmap_set_bit (r, to); > > > } > > > } > > > # in ira.c one would have to fixup the curly braces manually > > > PS: coccinelle seems to ruin the spaces before braces in the '+' even > > > though i have written them correctly according to GNU style.. > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] 2021-11-02 13:43 ` Richard Biener @ 2021-11-02 18:01 ` Richard Sandiford 2021-11-02 21:00 ` Bernhard Reutner-Fischer 1 sibling, 0 replies; 11+ messages in thread From: Richard Sandiford @ 2021-11-02 18:01 UTC (permalink / raw) To: Richard Biener via Gcc-patches; +Cc: Bernhard Reutner-Fischer, Richard Biener Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > On Mon, Nov 1, 2021 at 10:02 PM Bernhard Reutner-Fischer via > Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> >> On Mon, 1 Nov 2021 15:21:03 +0100 >> Aldy Hernandez <aldyh@redhat.com> wrote: >> >> > I'm not convinced this makes the code clearer to read, especially if >> > it's not on a critical path. But if you feel strongly, please submit >> > a patch ;-). >> >> No i don't feel strongly about it. >> Compiling e.g. -O2 ira.o >> # Overhead Samples Command Shared Object Symbol >> # ........ ............ ....... ............. ......................... >> # >> 100.00% 4197 cc1plus cc1plus [.] mark_reachable_blocks >> 100.00% 22000 cc1plus cc1plus [.] path_oracle::killing_def >> and the mark_elimination is reload. >> So it's not just a handful of calls saved but some. And it would be >> smaller code as it saves a call. Well maybe another day. > > Note that single bit set/clear are already implemented as test && set/clear. > Note that unfortunately the sbitmap bitmap_set/clear_bit overloads do not > return the previous state of the bit. +1 that it would good if the sbitmap versions behaved like the bitmap versions. Always low-key bothered me that they didn't do this when I hit it, but never got round to do anything about it... Bitmap operations consistently show up high in the profiles, so IMO using the return value of bitmap_set_bit and bitmap_clear_bit should be the preferred style. Not all uses are performance-critical, of course, but code tends to get copied around. Having all code do it the fast way reduces the risk that slow code gets copied to code that needs to be fast. :-) > Maybe providing bitmap_test_and_set_bit () and > bitmap_test_and_clear_bit () would be better (but note we currently > return true when the bit changed, not when it was set). Yeah, maybe it's just familiarity, but I find the: if (bitmap_set_bit (bb, i)) ...something changed... thing easier to follow than: if (!bitmap_test_and_set_bit (bb, i)) ...something changed... Thanks, Richard ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] 2021-11-02 13:43 ` Richard Biener 2021-11-02 18:01 ` Richard Sandiford @ 2021-11-02 21:00 ` Bernhard Reutner-Fischer 2021-11-03 8:01 ` Richard Biener 1 sibling, 1 reply; 11+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-02 21:00 UTC (permalink / raw) To: Richard Biener; +Cc: Aldy Hernandez, GCC patches On 2 November 2021 14:43:38 CET, Richard Biener <richard.guenther@gmail.com> wrote: >On Mon, Nov 1, 2021 at 10:02 PM Bernhard Reutner-Fischer via >Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> >> On Mon, 1 Nov 2021 15:21:03 +0100 >> Aldy Hernandez <aldyh@redhat.com> wrote: >> >> > I'm not convinced this makes the code clearer to read, especially if >> > it's not on a critical path. But if you feel strongly, please submit >> > a patch ;-). >> >> No i don't feel strongly about it. >> Compiling e.g. -O2 ira.o >> # Overhead Samples Command Shared Object Symbol >> # ........ ............ ....... ............. ......................... >> # >> 100.00% 4197 cc1plus cc1plus [.] mark_reachable_blocks >> 100.00% 22000 cc1plus cc1plus [.] path_oracle::killing_def >> and the mark_elimination is reload. >> So it's not just a handful of calls saved but some. And it would be >> smaller code as it saves a call. Well maybe another day. > >Note that single bit set/clear are already implemented as test && set/clear. >Note that unfortunately the sbitmap bitmap_set/clear_bit overloads do not >return the previous state of the bit. Maybe providing All 3 spots here, dse, ira and killing_def do not look at the bit but use it as a bool, fwiw. These 3 are the only users I (resp coccinelle) found who do the redundant bitmap_bit_p, bitmap_clear_bit. >bitmap_test_and_set_bit () and bitmap_test_and_clear_bit () would be >better (but note we currently return true when the bit changed, not when >it was set). A big portion of users of bitmap_bit_p could use a bitmap_bit_p that returns bool as is. The rest would know the bit they asked for, no? Hence bitmap_bit_p could be changed to return a bool rather easily. testb instead of testl on x86_64 fwiw. thanks, > >Richard. > >> thanks, >> > >> > Aldy >> > >> > On Mon, Nov 1, 2021 at 3:10 PM Bernhard Reutner-Fischer >> > <rep.dot.nop@gmail.com> wrote: >> > > >> > > On Thu, 28 Oct 2021 01:55:30 +0200 >> > > Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: >> > > >> > > > On Wed, 27 Oct 2021 20:13:21 +0200 >> > > > Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> > > >> > > > > @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) >> > > > > ptr->m_next = m_equiv.m_next; >> > > > > m_equiv.m_next = ptr; >> > > > > bitmap_ior_into (m_equiv.m_names, b); >> > > > > + >> > > > > + // Walk the relation list an remove SSA from any relations. >> > > > >> > > > s/an /and / >> > > > >> > > > > + if (!bitmap_bit_p (m_relations.m_names, v)) >> > > > > + return; >> > > > > + >> > > > > + bitmap_clear_bit (m_relations.m_names, v); >> > > > >> > > > IIRC bitmap_clear_bit returns true if the bit was set, false otherwise, >> > > > so should be used as if(!bitmap_clear_bit) above. >> > > >> > > > > + relation_chain **prev = &(m_relations.m_head); >> > > > >> > > > s/[()]// >> > > > thanks, >> > > >> > > There seems to be two other spots where a redundant bitmap_bit_p checks >> > > if we want to bitmap_clear_bit. In dse and ira. >> > > Like: >> > > $ cat ~/coccinelle/gcc_bitmap_bit_p-0.cocci ; echo EOF >> > > // replace redundant bitmap_bit_p() bitmap_clear_bit with the latter >> > > @ rule1 @ >> > > identifier fn; >> > > expression bitmap, bit; >> > > @@ >> > > >> > > fn(...) { >> > > <... >> > > ( >> > > -if (bitmap_bit_p (bitmap, bit)) >> > > +if (bitmap_clear_bit (bitmap, bit)) >> > > { >> > > ... >> > > - bitmap_clear_bit (bitmap, bit); >> > > ... >> > > } >> > > | >> > > -if (bitmap_bit_p (bitmap, bit)) >> > > +if (bitmap_clear_bit (bitmap, bit)) >> > > { >> > > ... >> > > } >> > > ... >> > > -bitmap_clear_bit (bitmap, bit); >> > > ) >> > > ...> >> > > } >> > > EOF >> > > $ find gcc/ -type f -a \( -name "*.c" -o -name "*.cc" \) -a \( ! -path "gcc/testsuite/*" -a ! -path "gcc/contrib/*" \) -exec spatch -sp_file ~/coccinelle/gcc_bitmap_bit_p-0.cocci --show-diff {} \; >> > > diff = >> > > --- gcc/dse.c >> > > +++ /tmp/cocci-output-1104419-443759-dse.c >> > > @@ -3238,9 +3238,8 @@ mark_reachable_blocks (sbitmap unreachab >> > > edge e; >> > > edge_iterator ei; >> > > >> > > - if (bitmap_bit_p (unreachable_blocks, bb->index)) >> > > + if (bitmap_clear_bit(unreachable_blocks, bb->index)) >> > > { >> > > - bitmap_clear_bit (unreachable_blocks, bb->index); >> > > FOR_EACH_EDGE (e, ei, bb->preds) >> > > { >> > > mark_reachable_blocks (unreachable_blocks, e->src); >> > > diff = >> > > --- gcc/ira.c >> > > +++ /tmp/cocci-output-1104678-d8679a-ira.c >> > > @@ -2944,17 +2944,15 @@ mark_elimination (int from, int to) >> > > FOR_EACH_BB_FN (bb, cfun) >> > > { >> > > r = DF_LR_IN (bb); >> > > - if (bitmap_bit_p (r, from)) >> > > + if (bitmap_clear_bit(r, from)) >> > > { >> > > - bitmap_clear_bit (r, from); >> > > bitmap_set_bit (r, to); >> > > } >> > > if (! df_live) >> > > continue; >> > > r = DF_LIVE_IN (bb); >> > > - if (bitmap_bit_p (r, from)) >> > > + if (bitmap_clear_bit(r, from)) >> > > { >> > > - bitmap_clear_bit (r, from); >> > > bitmap_set_bit (r, to); >> > > } >> > > } >> > > # in ira.c one would have to fixup the curly braces manually >> > > PS: coccinelle seems to ruin the spaces before braces in the '+' even >> > > though i have written them correctly according to GNU style.. >> > > >> > >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] 2021-11-02 21:00 ` Bernhard Reutner-Fischer @ 2021-11-03 8:01 ` Richard Biener 0 siblings, 0 replies; 11+ messages in thread From: Richard Biener @ 2021-11-03 8:01 UTC (permalink / raw) To: Bernhard Reutner-Fischer; +Cc: Aldy Hernandez, GCC patches On Tue, Nov 2, 2021 at 10:00 PM Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > > On 2 November 2021 14:43:38 CET, Richard Biener <richard.guenther@gmail.com> wrote: > >On Mon, Nov 1, 2021 at 10:02 PM Bernhard Reutner-Fischer via > >Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > >> > >> On Mon, 1 Nov 2021 15:21:03 +0100 > >> Aldy Hernandez <aldyh@redhat.com> wrote: > >> > >> > I'm not convinced this makes the code clearer to read, especially if > >> > it's not on a critical path. But if you feel strongly, please submit > >> > a patch ;-). > >> > >> No i don't feel strongly about it. > >> Compiling e.g. -O2 ira.o > >> # Overhead Samples Command Shared Object Symbol > >> # ........ ............ ....... ............. ......................... > >> # > >> 100.00% 4197 cc1plus cc1plus [.] mark_reachable_blocks > >> 100.00% 22000 cc1plus cc1plus [.] path_oracle::killing_def > >> and the mark_elimination is reload. > >> So it's not just a handful of calls saved but some. And it would be > >> smaller code as it saves a call. Well maybe another day. > > > >Note that single bit set/clear are already implemented as test && set/clear. > >Note that unfortunately the sbitmap bitmap_set/clear_bit overloads do not > >return the previous state of the bit. Maybe providing > > All 3 spots here, dse, ira and killing_def do not look at the bit but use it as a bool, fwiw. > These 3 are the only users I (resp coccinelle) found who do the redundant bitmap_bit_p, bitmap_clear_bit. > > >bitmap_test_and_set_bit () and bitmap_test_and_clear_bit () would be > >better (but note we currently return true when the bit changed, not when > >it was set). > > A big portion of users of bitmap_bit_p could use a bitmap_bit_p that returns bool as is. > The rest would know the bit they asked for, no? > Hence bitmap_bit_p could be changed to return a bool rather easily. testb instead of testl on x86_64 fwiw. > thanks, Yes, I think bitmap_bit_p returns [0, 1] already, 'int' is just some pre-bool standard return type... I'm going to test a patch changing the signature. Richard. > > > > >Richard. > > > >> thanks, > >> > > >> > Aldy > >> > > >> > On Mon, Nov 1, 2021 at 3:10 PM Bernhard Reutner-Fischer > >> > <rep.dot.nop@gmail.com> wrote: > >> > > > >> > > On Thu, 28 Oct 2021 01:55:30 +0200 > >> > > Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > >> > > > >> > > > On Wed, 27 Oct 2021 20:13:21 +0200 > >> > > > Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > >> > > > >> > > > > @@ -1306,6 +1307,24 @@ path_oracle::killing_def (tree ssa) > >> > > > > ptr->m_next = m_equiv.m_next; > >> > > > > m_equiv.m_next = ptr; > >> > > > > bitmap_ior_into (m_equiv.m_names, b); > >> > > > > + > >> > > > > + // Walk the relation list an remove SSA from any relations. > >> > > > > >> > > > s/an /and / > >> > > > > >> > > > > + if (!bitmap_bit_p (m_relations.m_names, v)) > >> > > > > + return; > >> > > > > + > >> > > > > + bitmap_clear_bit (m_relations.m_names, v); > >> > > > > >> > > > IIRC bitmap_clear_bit returns true if the bit was set, false otherwise, > >> > > > so should be used as if(!bitmap_clear_bit) above. > >> > > > >> > > > > + relation_chain **prev = &(m_relations.m_head); > >> > > > > >> > > > s/[()]// > >> > > > thanks, > >> > > > >> > > There seems to be two other spots where a redundant bitmap_bit_p checks > >> > > if we want to bitmap_clear_bit. In dse and ira. > >> > > Like: > >> > > $ cat ~/coccinelle/gcc_bitmap_bit_p-0.cocci ; echo EOF > >> > > // replace redundant bitmap_bit_p() bitmap_clear_bit with the latter > >> > > @ rule1 @ > >> > > identifier fn; > >> > > expression bitmap, bit; > >> > > @@ > >> > > > >> > > fn(...) { > >> > > <... > >> > > ( > >> > > -if (bitmap_bit_p (bitmap, bit)) > >> > > +if (bitmap_clear_bit (bitmap, bit)) > >> > > { > >> > > ... > >> > > - bitmap_clear_bit (bitmap, bit); > >> > > ... > >> > > } > >> > > | > >> > > -if (bitmap_bit_p (bitmap, bit)) > >> > > +if (bitmap_clear_bit (bitmap, bit)) > >> > > { > >> > > ... > >> > > } > >> > > ... > >> > > -bitmap_clear_bit (bitmap, bit); > >> > > ) > >> > > ...> > >> > > } > >> > > EOF > >> > > $ find gcc/ -type f -a \( -name "*.c" -o -name "*.cc" \) -a \( ! -path "gcc/testsuite/*" -a ! -path "gcc/contrib/*" \) -exec spatch -sp_file ~/coccinelle/gcc_bitmap_bit_p-0.cocci --show-diff {} \; > >> > > diff = > >> > > --- gcc/dse.c > >> > > +++ /tmp/cocci-output-1104419-443759-dse.c > >> > > @@ -3238,9 +3238,8 @@ mark_reachable_blocks (sbitmap unreachab > >> > > edge e; > >> > > edge_iterator ei; > >> > > > >> > > - if (bitmap_bit_p (unreachable_blocks, bb->index)) > >> > > + if (bitmap_clear_bit(unreachable_blocks, bb->index)) > >> > > { > >> > > - bitmap_clear_bit (unreachable_blocks, bb->index); > >> > > FOR_EACH_EDGE (e, ei, bb->preds) > >> > > { > >> > > mark_reachable_blocks (unreachable_blocks, e->src); > >> > > diff = > >> > > --- gcc/ira.c > >> > > +++ /tmp/cocci-output-1104678-d8679a-ira.c > >> > > @@ -2944,17 +2944,15 @@ mark_elimination (int from, int to) > >> > > FOR_EACH_BB_FN (bb, cfun) > >> > > { > >> > > r = DF_LR_IN (bb); > >> > > - if (bitmap_bit_p (r, from)) > >> > > + if (bitmap_clear_bit(r, from)) > >> > > { > >> > > - bitmap_clear_bit (r, from); > >> > > bitmap_set_bit (r, to); > >> > > } > >> > > if (! df_live) > >> > > continue; > >> > > r = DF_LIVE_IN (bb); > >> > > - if (bitmap_bit_p (r, from)) > >> > > + if (bitmap_clear_bit(r, from)) > >> > > { > >> > > - bitmap_clear_bit (r, from); > >> > > bitmap_set_bit (r, to); > >> > > } > >> > > } > >> > > # in ira.c one would have to fixup the curly braces manually > >> > > PS: coccinelle seems to ruin the spaces before braces in the '+' even > >> > > though i have written them correctly according to GNU style.. > >> > > > >> > > >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-11-03 13:25 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-27 18:13 [COMMITTED] Kill second order relations in the path solver Aldy Hernandez 2021-10-27 18:13 ` [COMMITTED] Reorder relation calculating code " Aldy Hernandez 2021-10-27 18:13 ` [COMMITTED] Kill known equivalences before a new assignment " Aldy Hernandez 2021-10-27 23:55 ` [COMMITTED] Kill second order relations " Bernhard Reutner-Fischer 2021-11-01 14:10 ` redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] Bernhard Reutner-Fischer 2021-11-01 14:21 ` Aldy Hernandez 2021-11-01 21:02 ` Bernhard Reutner-Fischer 2021-11-02 13:43 ` Richard Biener 2021-11-02 18:01 ` Richard Sandiford 2021-11-02 21:00 ` Bernhard Reutner-Fischer 2021-11-03 8:01 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).