Re: [PATCH] Fix expand_builtin_atomic_fetch_op for pre-op (PR80902)

[Segher Boessenkool <segher at kernel dot crashing dot org>]

On Fri, Jul 07, 2017 at 06:56:09AM -0500, Segher Boessenkool wrote:
> On Thu, Jun 22, 2017 at 10:59:05PM -0600, Jeff Law wrote:
> > On 05/28/2017 06:31 AM, Segher Boessenkool wrote:
> > > __atomic_add_fetch adds a value to some memory, and returns the result.
> > > If there is no direct support for this, expand_builtin_atomic_fetch_op
> > > is asked to implement this as __atomic_fetch_add (which returns the
> > > original value of the mem), followed by the addition.  Now, the
> > > __atomic_add_fetch could have been a tail call, but we shouldn't
> > > perform the __atomic_fetch_add as a tail call: following code would
> > > not be executed, and in fact thrown away because there is a barrier
> > > after tail calls.
> 
> > Hmmm.  I wonder if we have similar problems elsewhere.  For example
> > expand_builtin_int_roundingfn_2, stack_protect_epilogue,
> > expand_builtin_trap (though this one probably isn't broken in practice),
> > expand_ifn_atomic_compare_exchange_into_call.
> > 
> > OK, but please check the other instances where we call expand_call, then
> > continue generating code afterwards.  Fixing those can be a follow-up patch.
> 
> We certainly have similar problems elsewhere.
> 
> I'm doing tests detecting whenever we create dead code (right after a
> barrier); it finds a few things, mostly harmless, but there are quite
> a few places where we create dead code during expand.  This will take
> a while, but it will need to happen during stage 1...  I'm trying to
> fit it in :-/

The following patch bootstraps on powerpc64-linux, and builds cross to
*-linux, and builds Linux with it.  I'm not proposing it for inclusion
right now though; it is too disgusting, and at least it should only do
this with checking enabled.  It might be useful to someone though.  It
does complain about the PR80902 problem, at least.


Segher


diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index c9d8118..d424d91 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -3843,17 +3843,79 @@ expand_gimple_tailcall (basic_block bb, gcall *stmt, bool *can_fallthru)
   last = NEXT_INSN (last);
   gcc_assert (BARRIER_P (last));
 
+  bool fatal_error = false;
   *can_fallthru = false;
-  while (NEXT_INSN (last))
+  for (rtx_insn *insn = NEXT_INSN (last); insn; insn = NEXT_INSN (insn))
     {
       /* For instance an sqrt builtin expander expands if with
 	 sibcall in the then and label for `else`.  */
-      if (LABEL_P (NEXT_INSN (last)))
+      if (LABEL_P (insn))
+	break;
+
+      /* Complain if we would delete an insn that is not a move for the
+	 function return value.  */
+      if (NONDEBUG_INSN_P (insn))
+	{
+	  bool okay = false;
+	  rtx set = single_set (insn);
+	  if (set)
+	    {
+	      rtx dest = SET_DEST (set);
+	      rtx src = SET_SRC (set);
+	      if (GET_CODE (src) == SIGN_EXTEND
+		  || GET_CODE (src) == ZERO_EXTEND)
+		src = XEXP (src, 0);
+	      if ((OBJECT_P (dest) || SUBREG_P (dest))
+		  && (OBJECT_P (src) || SUBREG_P (src)))
+		okay = true;
+	      /* Not too bad so far...  but some weirder things are emitted
+		 as well, unfortunately.  */
+	      if (REG_P (dest) && REGNO (dest) == STACK_POINTER_REGNUM)
+		okay = true;
+	      /* mips does the following:  */
+	      if (REG_P (dest)
+		  && GET_CODE (src) == UNSPEC
+		  && XVECLEN (src, 0) == 1
+		  && REG_P (XVECEXP (src, 0, 0)))
+		okay = true;
+	      /* And at least tile* does this, followed by other code to
+		 calculate more stack addresses.  So let's ignore the rest
+		 of the code.  */
+	      if (GET_CODE (src) == PLUS
+		  && REG_P (dest)
+		  && REG_P (XEXP (src, 0))
+		  && IN_RANGE (REGNO (XEXP (src, 0)),
+			       FIRST_VIRTUAL_REGISTER,
+			       LAST_VIRTUAL_REGISTER))
+		break;
+	    }
+	  else if (GET_CODE (PATTERN (insn)) == CLOBBER)
+	    okay = true;
+	  if (!okay)
+	    fatal_error = true;
+	}
+    }
+
+  if (fatal_error)
+    {
+      fprintf (stderr, "DELETING:\n");
+      for (rtx_insn *insn = NEXT_INSN (last); insn; insn = NEXT_INSN (insn))
+	debug_rtx (insn);
+
+      gcc_assert (0);
+    }
+
+  for (rtx_insn *insn = NEXT_INSN (last); insn; insn = NEXT_INSN (last))
+    {
+      /* For instance an sqrt builtin expander expands if with
+	 sibcall in the then and label for `else`.  */
+      if (LABEL_P (insn))
 	{
 	  *can_fallthru = true;
 	  break;
 	}
-      delete_insn (NEXT_INSN (last));
+
+      delete_insn (insn);
     }
 
   e = make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), EDGE_ABNORMAL