[PATCH] expand: Fix handling of asm goto outputs vs. PHI argument adjustments [PR113921]

[PATCH] expand: Fix handling of asm goto outputs vs. PHI argument adjustments [PR113921]

[Jakub Jelinek]

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?

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

Re: [PATCH] expand: Fix handling of asm goto outputs vs. PHI argument adjustments [PR113921]

[Richard Biener]

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
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)