[PATCH][expr.c] PR 65358 Avoid clobbering partial argument during sibcall

[Kyrill Tkachov <kyrylo.tkachov@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

Hi all,

This patch fixes PR 65358. For details look at the excellent write-up
by Honggyu in bugzilla. The problem is that we're trying to pass a struct
partially on the stack and partially in regs during a tail-call optimisation
but the struct we're passing is also a partial incoming arg though the split
between stack and regs is different from its outgoing usage.

The emit_push_insn code ends up doing a block move for the on-stack part but
ends up overwriting the part that needs to be loaded into regs.
My first thought was to just load the regs part first and then do the stack
part but that doesn't work as multiple comments in that function indicate
(the block move being expanded to movmem or other functions being one of the
reasons).

My proposed solution is to detect when the overlap happens, find the
overlapping region and load it before the stack pushing into pseudos and
after the stack pushing is done move the overlapping values from the pseudos
into the hard argument regs that they're supposed to go.

That way this new functionality should only ever be triggered when there's
the overlap in this PR (causing wrong-code) and shouldn't affect codegen
anywhere else.

Bootstrapped and tested on arm-none-linux-gnueabihf, aarch64-none-linux-gnu
and x86_64-linux-gnu.

According to the PR this appears at least as far back 4.6 so this isn't a
regression on the release branches, but it is a wrong-code bug.

I'll let Honggyu upstream the testcase separately
(https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00984.html)

I'll be testing this on the 4.8 and 4.9 branches.
Thoughts on this approach?

Thanks,
Kyrill

2015-03-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR middle-end/65358
     * expr.c (memory_load_overlap): New function.
     (emit_push_insn): When pushing partial args to the stack would
     clobber the register part load the overlapping part into a pseudo
     and put it into the hard reg after pushing.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: expr.patch --]
[-- Type: text/x-patch; name=expr.patch, Size: 3995 bytes --]

commit 490c5f2074d76a2927afaea99e4dd0bacccb413c
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed Mar 18 13:42:37 2015 +0000

    [expr.c] PR 65358 Avoid clobbering partial argument during sibcall

diff --git a/gcc/expr.c b/gcc/expr.c
index dc13a14..d3b9156 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -4121,6 +4121,25 @@ emit_single_push_insn (machine_mode mode, rtx x, tree type)
 }
 #endif
 
+/* Add SIZE to X and check whether it's greater than Y.
+   If it is, return the constant amount by which it's greater or smaller.
+   If the two are not statically comparable (for example, X and Y contain
+   different registers) return -1.  This is used in expand_push_insn to
+   figure out if reading SIZE bytes from location X will end up reading from
+   location Y.  */
+
+static int
+memory_load_overlap (rtx x, rtx y, HOST_WIDE_INT size)
+{
+  rtx tmp = plus_constant (Pmode, x, size);
+  rtx sub = simplify_gen_binary (MINUS, Pmode, tmp, y);
+
+  if (!CONST_INT_P (sub))
+    return -1;
+
+  return INTVAL (sub);
+}
+
 /* Generate code to push X onto the stack, assuming it has mode MODE and
    type TYPE.
    MODE is redundant except when X is a CONST_INT (since they don't
@@ -4179,6 +4198,10 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size,
 
   xinner = x;
 
+  int nregs = partial / UNITS_PER_WORD;
+  rtx *tmp_regs = NULL;
+  int overlapping = 0;
+
   if (mode == BLKmode
       || (STRICT_ALIGNMENT && align < GET_MODE_ALIGNMENT (mode)))
     {
@@ -4309,6 +4332,35 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size,
 	     PARM_BOUNDARY.  Assume the caller isn't lying.  */
 	  set_mem_align (target, align);
 
+	  /* If part should go in registers and pushing to that part would
+	     overwrite some of the values that need to go into regs, load the
+	     overlapping values into temporary pseudos to be moved into the hard
+	     regs at the end after the stack pushing has completed.
+	     We cannot load them directly into the hard regs here because
+	     they can be clobbered by the block move expansions.
+	     See PR 65358.  */
+
+	  if (partial > 0 && reg != 0 && mode == BLKmode
+	      && GET_CODE (reg) != PARALLEL)
+	    {
+	      overlapping = memory_load_overlap (XEXP (x, 0), temp, partial);
+	      if (overlapping > 0)
+	        {
+		  gcc_assert (overlapping % UNITS_PER_WORD == 0);
+		  overlapping /= UNITS_PER_WORD;
+
+		  tmp_regs = XALLOCAVEC (rtx, overlapping);
+
+		  for (int i = 0; i < overlapping; i++)
+		    tmp_regs[i] = gen_reg_rtx (word_mode);
+
+		  for (int i = 0; i < overlapping; i++)
+		    emit_move_insn (tmp_regs[i],
+				    operand_subword_force (target, i, mode));
+	        }
+	      else
+		overlapping = 0;
+	    }
 	  emit_block_move (target, xinner, size, BLOCK_OP_CALL_PARM);
 	}
     }
@@ -4411,9 +4463,8 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size,
 	}
     }
 
-  /* If part should go in registers, copy that part
-     into the appropriate registers.  Do this now, at the end,
-     since mem-to-mem copies above may do function calls.  */
+  /* Move the partial arguments into the registers and any overlapping
+     values that we moved into the pseudos in tmp_regs.  */
   if (partial > 0 && reg != 0)
     {
       /* Handle calls that pass values in multiple non-contiguous locations.
@@ -4421,9 +4472,15 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size,
       if (GET_CODE (reg) == PARALLEL)
 	emit_group_load (reg, x, type, -1);
       else
-	{
+        {
 	  gcc_assert (partial % UNITS_PER_WORD == 0);
-	  move_block_to_reg (REGNO (reg), x, partial / UNITS_PER_WORD, mode);
+	  move_block_to_reg (REGNO (reg), x, nregs - overlapping, mode);
+
+	  for (int i = 0; i < overlapping; i++)
+	    emit_move_insn (gen_rtx_REG (word_mode, REGNO (reg)
+						    + nregs - overlapping + i),
+			    tmp_regs[i]);
+
 	}
     }