Message ID | Zc3Fzn1MwlbH8wY8@tucnak |
---|---|
State | New |
Headers | show |
Series | expand: Fix handling of asm goto outputs vs. PHI argument adjustments [PR113921] | expand |
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 <jakub@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 > >
--- 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 (); +}