On Thu, Mar 5, 2020 at 5:49 PM J.W. Jagersma <jwjager...@gmail.com> wrote:
>
> The following patch extends the generation of exception handling
> information to cover volatile asms too. This was already mostly
> implemented, and only very minor changes are required in order to make
> it work.
>
> The change in rewrite_stmt is necessary because it inserts debug
> statements after the asm, and this causes the gimple verification pass
> to fail. This code is copied from maybe_register_def which does the
> same thing. Alternatively the verification routines could be made to
> ignore debug statements at the end of a block.
>
> gcc/
> 2020-03-05 Jan W. Jagersma <jwjager...@gmail.com>
>
> PR inline-asm/93981
> * tree-cfg.c (make_edges_bb): Call make_eh_edges for case
> GIMPLE_ASM.
> * tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
> * tree-into-ssa.c (rewrite_stmt): For bb-ending stmts, insert
> debug notes on the fallthrough edge. Code and comments copied
> verbatim from maybe_register_def.
> ---
> gcc/tree-cfg.c | 2 ++
> gcc/tree-eh.c | 3 +++
> gcc/tree-into-ssa.c | 38 +++++++++++++++++++++++++++++++++++++-
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index f7b817d94e6..c21a7978493 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -913,6 +913,8 @@ make_edges_bb (basic_block bb, struct omp_region
> **pcur_region, int *pomp_index)
> break;
>
> case GIMPLE_ASM:
> + if (stmt_can_throw_internal (cfun, last))
> + make_eh_edges (last);
> make_gimple_asm_edges (bb);
> fallthru = true;
> break;
> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
> index 2a409dcaffe..8314db00922 100644
> --- a/gcc/tree-eh.c
> +++ b/gcc/tree-eh.c
> @@ -2077,6 +2077,9 @@ lower_eh_constructs_2 (struct leh_state *state,
> gimple_stmt_iterator *gsi)
> DECL_GIMPLE_REG_P (tmp) = 1;
> gsi_insert_after (gsi, s, GSI_SAME_STMT);
> }
> + /* FALLTHRU */
> +
> + case GIMPLE_ASM:
> /* Look for things that can throw exceptions, and record them. */
> if (state->cur_region && stmt_could_throw_p (cfun, stmt))
> {
The part you skip here for ASMs means that for ASM outputs we do
not represent their values correctly on EH edges (and we eventually
never will be able to since we don't know exactly at which point the
actual assembly throws). So I think this "feature" needs to be documented
appropriately and eventually only ASMs without outputs made allowed
to throw?
> diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
> index 6528acac31a..03bc1d52cfa 100644
> --- a/gcc/tree-into-ssa.c
> +++ b/gcc/tree-into-ssa.c
> @@ -1415,7 +1415,43 @@ rewrite_stmt (gimple_stmt_iterator *si)
> if (tracked_var)
> {
> gimple *note = gimple_build_debug_bind (tracked_var, name, stmt);
> - gsi_insert_after (si, note, GSI_SAME_STMT);
> + /* If stmt ends the bb, insert the debug stmt on the single
> + non-EH edge from the stmt. */
> + if (gsi_one_before_end_p (*si) && stmt_ends_bb_p (stmt))
> + {
> + basic_block bb = gsi_bb (*si);
> + edge_iterator ei;
> + edge e, ef = NULL;
> + FOR_EACH_EDGE (e, ei, bb->succs)
> + if (!(e->flags & EDGE_EH))
I think this needs to check for abnormal edges as well to cover
the case of
i = setjmp ();
which means doing !(e->flags & EDGE_COMPLEX) and adjusting the
comment.
> + {
> + gcc_checking_assert (!ef);
> + ef = e;
> + }
> + /* If there are other predecessors to ef->dest, then
> + there must be PHI nodes for the modified
> + variable, and therefore there will be debug bind
> + stmts after the PHI nodes. The debug bind notes
> + we'd insert would force the creation of a new
> + block (diverging codegen) and be redundant with
> + the post-PHI bind stmts, so don't add them.
> +
> + As for the exit edge, there wouldn't be redundant
> + bind stmts, but there wouldn't be a PC to bind
> + them to either, so avoid diverging the CFG. */
> + if (ef && single_pred_p (ef->dest)
> + && ef->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
> + {
> + /* If there were PHI nodes in the node, we'd
> + have to make sure the value we're binding
> + doesn't need rewriting. But there shouldn't
> + be PHI nodes in a single-predecessor block,
> + so we just add the note. */
> + gsi_insert_on_edge_immediate (ef, note);
> + }
it would be nice to elide building of the debug bind if we don't
end up inserting it.
Otherwise the patch looks reasonable. It lacks a testcase though,
I think a target specific testcase can be constructed easily.
Thanks,
Richard.
> + }
> + else
> + gsi_insert_after (si, note, GSI_SAME_STMT);
> }
> }
> }
> --
> 2.25.1
>
