On Thu, 14 Mar 2019, Jakub Jelinek wrote:
> On Thu, Mar 14, 2019 at 03:10:59PM +0100, Richard Biener wrote:
> > I've added a testcase.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK for trunk
> > or should it wait for GCC10?
>
> I meant something like following where we'd clean it up everything right
> away after we DCE some returns_twice calls.
>
> Except that doesn't work well, because the temporary
> cfun->calls_setjmp = true;
> hack not only allows gimple_purge_dead_abnormal_call_edges through the
> condition your patch was removing, but also makes then
> call_can_make_abnormal_goto
> and stmt_can_make_abnormal_goto to return true. So, maybe on top of this
> patch add a bool force argument to gimple_purge_dead_abnormal_call_edges
> and gimple_purge_all_dead_abnormal_call_edges and instead of the temporary
> cfun->calls_setjmp = true;
Well, the check in gimple_purge_dead_abnormal_call_edges is simply
a (IMHO) premature optimization. Passes shouldn't call the function
w/o a good reason anyways. So my patch removing the guard there
solves this particular issue, no?
> in this patch pass true as force. Plus, if we lost the last returns_twice
> call and cfun->has_nonlocal_decl isn't set either, instead of calling
> gimple_purge_all_dead_abnormal_call_edges just on the selected bbs call
> gimple_purge_dead_abnormal_call_edges with force on on all the bbs
> because if there are no nonlocal labels and no cfun->calls_setjmp used to be
> set and is no longer set, then we don't need any AB edges from any calls?
Yes.
> --- gcc/tree-ssa-dce.c.jj 2019-01-10 11:43:17.044334047 +0100
> +++ gcc/tree-ssa-dce.c 2019-03-14 17:35:47.158038514 +0100
> @@ -1201,6 +1201,8 @@ eliminate_unnecessary_stmts (void)
> gimple *stmt;
> tree call;
> vec<basic_block> h;
> + bool calls_setjmp = cfun->calls_setjmp;
> + bitmap need_ab_cleanup = NULL;
>
> if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, "\nEliminating unnecessary statements:\n");
> @@ -1287,6 +1289,14 @@ eliminate_unnecessary_stmts (void)
> }
> if (!is_gimple_debug (stmt))
> something_changed = true;
> + if (calls_setjmp
> + && is_gimple_call (stmt)
> + && (gimple_call_flags (stmt) & ECF_RETURNS_TWICE))
> + {
> + if (need_ab_cleanup == NULL)
> + need_ab_cleanup = BITMAP_ALLOC (NULL);
> + bitmap_set_bit (need_ab_cleanup, bb->index);
> + }
This is equivalent to checking whether cfun->calls_setjmp is set
after this loop (and cheaper).
> remove_dead_stmt (&gsi, bb);
> }
> else if (is_gimple_call (stmt))
> @@ -1358,6 +1368,17 @@ eliminate_unnecessary_stmts (void)
>
> h.release ();
>
> + if (need_ab_cleanup)
> + {
> + bool saved_calls_setjmp = cfun->calls_setjmp;
> + cfun->calls_setjmp = true;
> + if (gimple_purge_all_dead_abnormal_call_edges (need_ab_cleanup))
> + cfg_altered = true;
also as you said it's incomplete since also functions !returns_twice
may no longer have the need for abnormal edges.
Note other passes have the very same issue (they just don't re-compute
->calls_setjmp). Eventually we can make cfg-cleanup re-compute that
for us given it looks at all BBs last stmt anyways (and only those
are relevant).
That said, I think we can go with my patch for GCC 9 and defer a more
complete and elaborate solution to GCC 10 (where I'd still prefer
sth simple).
What do you think?
Richard.
> + cfun->calls_setjmp = saved_calls_setjmp;
> +
> + BITMAP_FREE (need_ab_cleanup);
> + }
> +
> /* Since we don't track liveness of virtual PHI nodes, it is possible that
> we
> rendered some PHI nodes unreachable while they are still in use.
> Mark them for renaming. */
>
>
> Jakub
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
