| Message ID | 20211101151027.06945d6a@nbbrfq |
|---|---|
| State | New |
| Headers | show |
| Series | redundant bitmap_bit_p followed by bitmap_clear_bit [was: Re: [COMMITTED] Kill second order relations in the path solver.] | expand |
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.. >
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.. > > >
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.. > > > > > >
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
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.. >> > > >> > >>
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.. > >> > > > >> > > >> >
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); } }