Message ID | 20220215144427.4F6D713C9F@imap2.suse-dmz.suse.de |
---|---|
State | New |
Headers | show |
Series | tree-optimization/96881 - CD-DCE and CLOBBERs | expand |
> @@ -1272,7 +1275,7 @@ maybe_optimize_arith_overflow (gimple_stmt_iterator *gsi, > contributes nothing to the program, and can be deleted. */ > > static bool > -eliminate_unnecessary_stmts (void) > +eliminate_unnecessary_stmts (bool aggressive) > { > bool something_changed = false; > basic_block bb; > @@ -1366,7 +1369,9 @@ eliminate_unnecessary_stmts (void) > break; > } > } > - if (!dead) > + if (!dead > + && (!aggressive > + || bitmap_bit_p (visited_control_parents, bb->index))) It seems to me that it may be worth to consider case where visited_control_parents is 0 while all basic blocks in the CD relation are live for different reasons. I suppose this can happen in more complex CFGs when the other arms of conditionals are live... Honza > { > bitmap_clear (debug_seen); > continue; > @@ -1876,7 +1881,7 @@ perform_tree_ssa_dce (bool aggressive) > propagate_necessity (aggressive); > BITMAP_FREE (visited); > > - something_changed |= eliminate_unnecessary_stmts (); > + something_changed |= eliminate_unnecessary_stmts (aggressive); > something_changed |= cfg_altered; > > /* We do not update postdominators, so free them unconditionally. */ > -- > 2.34.1
On Tue, 15 Feb 2022, Jan Hubicka wrote: > > @@ -1272,7 +1275,7 @@ maybe_optimize_arith_overflow (gimple_stmt_iterator *gsi, > > contributes nothing to the program, and can be deleted. */ > > > > static bool > > -eliminate_unnecessary_stmts (void) > > +eliminate_unnecessary_stmts (bool aggressive) > > { > > bool something_changed = false; > > basic_block bb; > > @@ -1366,7 +1369,9 @@ eliminate_unnecessary_stmts (void) > > break; > > } > > } > > - if (!dead) > > + if (!dead > > + && (!aggressive > > + || bitmap_bit_p (visited_control_parents, bb->index))) > > It seems to me that it may be worth to consider case where > visited_control_parents is 0 while all basic blocks in the CD relation > are live for different reasons. I suppose this can happen in more > complex CFGs when the other arms of conditionals are live... It's a bit difficult to do in this place though since we might already have altered those blocks (and we need to check not for the block being live but for its control stmt). I suppose we could use the last_stmt_necessary bitmap. I'll do some statistics to see whether this helps. Richard.
On Thu, 17 Feb 2022, Richard Biener wrote: > On Tue, 15 Feb 2022, Jan Hubicka wrote: > > > > @@ -1272,7 +1275,7 @@ maybe_optimize_arith_overflow (gimple_stmt_iterator *gsi, > > > contributes nothing to the program, and can be deleted. */ > > > > > > static bool > > > -eliminate_unnecessary_stmts (void) > > > +eliminate_unnecessary_stmts (bool aggressive) > > > { > > > bool something_changed = false; > > > basic_block bb; > > > @@ -1366,7 +1369,9 @@ eliminate_unnecessary_stmts (void) > > > break; > > > } > > > } > > > - if (!dead) > > > + if (!dead > > > + && (!aggressive > > > + || bitmap_bit_p (visited_control_parents, bb->index))) > > > > It seems to me that it may be worth to consider case where > > visited_control_parents is 0 while all basic blocks in the CD relation > > are live for different reasons. I suppose this can happen in more > > complex CFGs when the other arms of conditionals are live... > > It's a bit difficult to do in this place though since we might already > have altered those blocks (and we need to check not for the block being > live but for its control stmt). I suppose we could use the > last_stmt_necessary bitmap. I'll do some statistics to see whether > this helps. So it does help. The visited_control_parents catches 44010 from 44033 candidates and the remaining 23 are catched by doing EXECUTE_IF_SET_IN_BITMAP (cd->get_edges_dependent_on (bb->index), 0, edge_number, bi) { basic_block cd_bb = cd->get_edge_src (edge_number); if (cd_bb != bb && !bitmap_bit_p (last_stmt_necessary, cd_bb->index)) { dead = true; break; } } in addition to that simple check. That means for all files in gcc/ the patch would be a no-op but it still fixes the problematical case. I'll put an adjusted patch to testing. Richard.
diff --git a/gcc/testsuite/g++.dg/torture/pr96881-1.C b/gcc/testsuite/g++.dg/torture/pr96881-1.C new file mode 100644 index 00000000000..1c182e6a8b3 --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr96881-1.C @@ -0,0 +1,37 @@ +/* { dg-do run } */ + +struct S { int s; ~S () {} } s; + +void __attribute__((noipa)) +foo (struct S *s, int flag) +{ + s->s = 1; + // We have to makes sure to not make the inlined CLOBBER + // unconditional but we have to remove it to be able + // to elide the branch + if (!flag) + return; + s->~S(); +} + +void __attribute__((noipa)) +bar (struct S *s, int flag) +{ + s->s = 1; + // CD-DCE chooses an arbitrary path, try to present it + // with all variants + if (flag) + s->~S(); +} + +int +main () +{ + foo (&s, 0); + if (s.s != 1) + __builtin_abort (); + bar (&s, 0); + if (s.s != 1) + __builtin_abort (); + return 0; +} diff --git a/gcc/testsuite/g++.dg/torture/pr96881-2.C b/gcc/testsuite/g++.dg/torture/pr96881-2.C new file mode 100644 index 00000000000..35c788791e8 --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr96881-2.C @@ -0,0 +1,37 @@ +/* { dg-do run } */ + +struct S { int s; ~S () {} } s; + +void __attribute__((noipa)) +foo (int flag) +{ + s.s = 1; + // We have to makes sure to not make the inlined CLOBBER + // unconditional but we have to remove it to be able + // to elide the branch + if (!flag) + return; + s.~S(); +} + +void __attribute__((noipa)) +bar (int flag) +{ + s.s = 1; + // CD-DCE chooses an arbitrary path, try to present it + // with all variants + if (flag) + s.~S(); +} + +int +main () +{ + foo (0); + if (s.s != 1) + __builtin_abort (); + bar (0); + if (s.s != 1) + __builtin_abort (); + return 0; +} diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc index f1034878eaf..9aefdc1c8d3 100644 --- a/gcc/tree-ssa-dce.cc +++ b/gcc/tree-ssa-dce.cc @@ -284,7 +284,10 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive) break; case GIMPLE_ASSIGN: - if (gimple_clobber_p (stmt)) + /* Mark indirect CLOBBERs to be lazily removed if their SSA operands + do not prevail. That also makes control flow leading to them + not necessary in aggressive mode. */ + if (gimple_clobber_p (stmt) && !zero_ssa_operands (stmt, SSA_OP_USE)) return; break; @@ -1272,7 +1275,7 @@ maybe_optimize_arith_overflow (gimple_stmt_iterator *gsi, contributes nothing to the program, and can be deleted. */ static bool -eliminate_unnecessary_stmts (void) +eliminate_unnecessary_stmts (bool aggressive) { bool something_changed = false; basic_block bb; @@ -1366,7 +1369,9 @@ eliminate_unnecessary_stmts (void) break; } } - if (!dead) + if (!dead + && (!aggressive + || bitmap_bit_p (visited_control_parents, bb->index))) { bitmap_clear (debug_seen); continue; @@ -1876,7 +1881,7 @@ perform_tree_ssa_dce (bool aggressive) propagate_necessity (aggressive); BITMAP_FREE (visited); - something_changed |= eliminate_unnecessary_stmts (); + something_changed |= eliminate_unnecessary_stmts (aggressive); something_changed |= cfg_altered; /* We do not update postdominators, so free them unconditionally. */