On Thu, 15 Feb 2024, Jakub Jelinek wrote:
> Hi!
>
> The Linux kernel and the following testcase distilled from it is
> miscompiled, because tree-outof-ssa.cc (eliminate_phi) emits some
> fixups on some of the edges (but doesn't commit edge insertions).
> Later expand_asm_stmt emits further instructions on the same edge.
> Now the problem is that expand_asm_stmt uses insert_insn_on_edge
> to add its own fixups, but that function appends to the existing
> sequence on the edge if any. And the bug triggers when the
> fixup sequence emitted by eliminate_phi uses a pseudo which the
> fixup sequence emitted by expand_asm_stmt later on sets.
> So, we end up with
> (set (reg A) (asm_operands ...))
> and on one of the edges queued sequence
> (set (reg C) (reg B)) // added by eliminate_phi
> (set (reg B) (reg A)) // added by expand_asm_stmt
> That is wrong, what we emit by expand_asm_stmt needs to be as close
> to the asm_operands as possible (they aren't known until expand_asm_stmt
> is called, the PHI fixup code assumes it is reg B which holds the right
> value) and the PHI adjustments need to be done after it.
>
> So, the following patch introduces a prepend_insn_to_edge function and
> uses it from expand_asm_stmt, so that we queue
> (set (reg B) (reg A)) // added by expand_asm_stmt
> (set (reg C) (reg B)) // added by eliminate_phi
> instead and so the value from the asm_operands output propagates correctly
> to the PHI result.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Thanks,
Richard.
> I think we need to backport it to all release branches (fortunately
> non-supported compilers aren't affected because GCC 11 was the first one
> to support asm goto with outputs), in cfgexpand.cc it won't apply cleanly
> due to the PR113415 fix, but manually applying it there will work.
>
> 2024-02-15 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/113921
> * cfgrtl.h (prepend_insn_to_edge): New declaration.
> * cfgrtl.cc (insert_insn_on_edge): Clarify behavior in function
> comment.
> (prepend_insn_to_edge): New function.
> * cfgexpand.cc (expand_asm_stmt): Use prepend_insn_to_edge instead of
> insert_insn_on_edge.
>
> * gcc.target/i386/pr113921.c: New test.
>
> --- gcc/cfgrtl.h.jj 2024-01-03 11:51:42.576577897 +0100
> +++ gcc/cfgrtl.h 2024-02-14 21:19:13.029797669 +0100
> @@ -38,6 +38,7 @@ extern edge try_redirect_by_replacing_ju
> extern void emit_barrier_after_bb (basic_block bb);
> extern basic_block force_nonfallthru_and_redirect (edge, basic_block, rtx);
> extern void insert_insn_on_edge (rtx, edge);
> +extern void prepend_insn_to_edge (rtx, edge);
> extern void commit_one_edge_insertion (edge e);
> extern void commit_edge_insertions (void);
> extern void print_rtl_with_bb (FILE *, const rtx_insn *, dump_flags_t);
> --- gcc/cfgrtl.cc.jj 2024-01-03 11:51:28.900767705 +0100
> +++ gcc/cfgrtl.cc 2024-02-14 21:19:24.036651779 +0100
> @@ -25,7 +25,7 @@ along with GCC; see the file COPYING3.
> - CFG-aware instruction chain manipulation
> delete_insn, delete_insn_chain
> - Edge splitting and committing to edges
> - insert_insn_on_edge, commit_edge_insertions
> + insert_insn_on_edge, prepend_insn_to_edge, commit_edge_insertions
> - CFG updating after insn simplification
> purge_dead_edges, purge_all_dead_edges
> - CFG fixing after coarse manipulation
> @@ -1966,7 +1966,8 @@ rtl_split_edge (edge edge_in)
>
> /* Queue instructions for insertion on an edge between two basic blocks.
> The new instructions and basic blocks (if any) will not appear in the
> - CFG until commit_edge_insertions is called. */
> + CFG until commit_edge_insertions is called. If there are already
> + queued instructions on the edge, PATTERN is appended to them. */
>
> void
> insert_insn_on_edge (rtx pattern, edge e)
> @@ -1984,6 +1985,25 @@ insert_insn_on_edge (rtx pattern, edge e
>
> e->insns.r = get_insns ();
> end_sequence ();
> +}
> +
> +/* Like insert_insn_on_edge, but if there are already queued instructions
> + on the edge, PATTERN is prepended to them. */
> +
> +void
> +prepend_insn_to_edge (rtx pattern, edge e)
> +{
> + /* We cannot insert instructions on an abnormal critical edge.
> + It will be easier to find the culprit if we die now. */
> + gcc_assert (!((e->flags & EDGE_ABNORMAL) && EDGE_CRITICAL_P (e)));
> +
> + start_sequence ();
> +
> + emit_insn (pattern);
> + emit_insn (e->insns.r);
> +
> + e->insns.r = get_insns ();
> + end_sequence ();
> }
>
> /* Update the CFG for the instructions queued on edge E. */
> --- gcc/cfgexpand.cc.jj 2024-02-10 11:25:09.995474027 +0100
> +++ gcc/cfgexpand.cc 2024-02-14 21:27:23.219300727 +0100
> @@ -3687,7 +3687,7 @@ expand_asm_stmt (gasm *stmt)
> copy = get_insns ();
> end_sequence ();
> }
> - insert_insn_on_edge (copy, e);
> + prepend_insn_to_edge (copy, e);
> }
> }
> }
> --- gcc/testsuite/gcc.target/i386/pr113921.c.jj 2024-02-14
> 21:21:15.194178515 +0100
> +++ gcc/testsuite/gcc.target/i386/pr113921.c 2024-02-14 21:20:52.745476040
> +0100
> @@ -0,0 +1,20 @@
> +/* PR middle-end/113921 */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +__attribute__((noipa)) long
> +foo (void)
> +{
> + long v;
> + asm volatile goto ("jmp %l2" : "=r" (v) : "0" (27) : : lab);
> + return v;
> +lab:
> + return 42;
> +}
> +
> +int
> +main ()
> +{
> + if (foo () != 42)
> + __builtin_abort ();
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
