RE: [Patch, MIPS] Patch for PR 68273 (user aligned variable arguments)

[Steve Ellcey <sellcey at imgtec dot com>]

On Wed, 2016-02-24 at 13:46 -0800, Matthew Fortune wrote:

> Thanks for enumerating all the cases. I'd not looked at all of them. I
> do agree that we need a fix given the existing inconsistencies.
> 
> One question I have is where does an over aligned argument get pushed
> to the stack with the patch in place. I.e. when taking its address, is
> the alignment achieved up to the limit of stack alignment or do they now
> only get register-size alignment? If the former then the idea that
> argument passing is defined as a structure in memory with the first
> portion in registers will no longer hold true. Not sure if that is a
> problem.

Passing arguments on the stack is not affected by this change, except
that we may start using memory at a different argument because of
changes in the number of registers we use to pass arguments.  The extra
alignment was already being ignored for arguments passed on the stack
and was only affecting register arguments.  So we are now more
consistent in terms of argument passing looking like a structure.  Those
structure 'fields' now do not include the user defined alignment for
register args or memory args.  An example:

typedef int alignedint __attribute__((aligned(8)));
int foo1 (int a, int b, int c, int d, int e, int x, alignedint y, int z)
{ return a+b+c+d+e+x+y+z; }

e, x, y, and z were being passed at offsets 16, 20, 24, and 28 before
this change and after this change.

> > diff --git a/gcc/testsuite/gcc.dg/pr48335-2.c
> > b/gcc/testsuite/gcc.dg/pr48335-2.c
> > index a37c079..f56b6fd 100644
> > --- a/gcc/testsuite/gcc.dg/pr48335-2.c
> > +++ b/gcc/testsuite/gcc.dg/pr48335-2.c
> > @@ -1,6 +1,7 @@
> >  /* PR middle-end/48335 */
> >  /* { dg-do compile } */
> >  /* { dg-options "-O2 -fno-tree-sra" } */
> > +/* { dg-options "-O2 -fno-tree-sra -mno-warn-aligned-args" { target
> > mips*-*-* } } */
> 
> Presumably this means that both under alignment of 64-bit types and over
> alignment of < 64-bit types are affected? The explicit test cases do not
> cover an under aligned long long case which would be good to cover.

Yes, the under alignment of long long's is affected.  I have added a new
testcase (pr68273-3.c) to check that.

Here is a new version of the patch with the invoke.texi text, the new
testcase, and the style changes you pointed out.

Steve Ellcey
sellcey@imgtec.com


2016-02-24  Steve Ellcey  <sellcey@imgtec.com>

	PR target/68273
	* config/mips/mips.opt (mwarn-aligned-args): New flag.
	* config/mips/mips.h (mips_args): Add new field.
	* config/mips/mips.c (mips_internal_function_arg): New function.
	(mips_function_incoming_arg): New function.
	(mips_old_function_arg_boundary): New function.
	(mips_function_arg): Rewrite to use mips_internal_function_arg.
	(mips_function_arg_boundary): Fix argument alignment.
	(TARGET_FUNCTION_INCOMING_ARG): New define.
	* doc/invoke.texi (MIPS Options): Document new flag.


diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 5af3d1e..ea4733b 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -1124,6 +1124,7 @@ static const struct mips_rtx_cost_data
 static rtx mips_find_pic_call_symbol (rtx_insn *, rtx, bool);
 static int mips_register_move_cost (machine_mode, reg_class_t,
 				    reg_class_t);
+static unsigned int mips_old_function_arg_boundary (machine_mode, const_tree);
 static unsigned int mips_function_arg_boundary (machine_mode, const_tree);
 static machine_mode mips_get_reg_raw_mode (int regno);
 
@@ -5459,11 +5460,11 @@ mips_strict_argument_naming (cumulative_args_t ca ATTRIBUTE_UNUSED)
   return !TARGET_OLDABI;
 }
 
-/* Implement TARGET_FUNCTION_ARG.  */
+/* Used to implement TARGET_FUNCTION_ARG and TARGET_FUNCTION_INCOMING_ARG.  */
 
 static rtx
-mips_function_arg (cumulative_args_t cum_v, machine_mode mode,
-		   const_tree type, bool named)
+mips_internal_function_arg (cumulative_args_t cum_v, machine_mode mode,
+			    const_tree type, bool named)
 {
   CUMULATIVE_ARGS *cum = get_cumulative_args (cum_v);
   struct mips_arg_info info;
@@ -5586,6 +5587,54 @@ mips_function_arg (cumulative_args_t cum_v, machine_mode mode,
   return gen_rtx_REG (mode, mips_arg_regno (&info, TARGET_HARD_FLOAT));
 }
 
+/* Implement TARGET_FUNCTION_ARG.  */
+
+static rtx
+mips_function_arg (cumulative_args_t cum_v, machine_mode mode,
+		   const_tree type, bool named)
+{
+  bool doubleword_aligned_p = (mips_function_arg_boundary (mode, type)
+ 			  > BITS_PER_WORD);
+  bool old_doubleword_aligned_p = (mips_old_function_arg_boundary (mode, type)
+			      > BITS_PER_WORD);
+  CUMULATIVE_ARGS *cum = get_cumulative_args (cum_v);
+
+  if (doubleword_aligned_p != old_doubleword_aligned_p
+      && mips_warn_aligned_args && !cum->aligned_arg_warning_given)
+    {
+      warning (0, "argument %d in the call may be passed in a manner"
+	       " incompatible with previous GCC versions",
+	       cum->arg_number + 1);
+      cum->aligned_arg_warning_given = 1;
+    }
+
+  return mips_internal_function_arg (cum_v, mode, type, named);
+}
+
+/* Implement TARGET_FUNCTION_INCOMING_ARG.  */
+
+static rtx
+mips_function_incoming_arg (cumulative_args_t cum_v, machine_mode mode,
+			    const_tree type, bool named)
+{
+  bool doubleword_aligned_p = (mips_function_arg_boundary (mode, type)
+ 			  > BITS_PER_WORD);
+  bool old_doubleword_aligned_p = (mips_old_function_arg_boundary (mode, type)
+			      > BITS_PER_WORD);
+  CUMULATIVE_ARGS *cum = get_cumulative_args (cum_v);
+
+  if (doubleword_aligned_p != old_doubleword_aligned_p
+      && mips_warn_aligned_args && !cum->aligned_arg_warning_given)
+    {
+      warning (0, "parameter %d of function %q+F may be passed in a manner"
+	       " incompatible with previous GCC versions",
+	       cum->arg_number + 1, current_function_decl);
+      cum->aligned_arg_warning_given = 1;
+    }
+
+  return mips_internal_function_arg (cum_v, mode, type, named);
+}
+
 /* Implement TARGET_FUNCTION_ARG_ADVANCE.  */
 
 static void
@@ -5635,6 +5684,23 @@ mips_arg_partial_bytes (cumulative_args_t cum,
   return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
 }
 
+/* This is the old TARGET_FUNCTION_ARG_BOUNDARY, we use it to see if there
+   is a change from the new one so we can warn users.  */
+
+static unsigned int
+mips_old_function_arg_boundary (machine_mode mode, const_tree type)
+{
+  unsigned int alignment;
+
+  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
+
+  if (alignment < PARM_BOUNDARY)
+    alignment = PARM_BOUNDARY;
+  if (alignment > STACK_BOUNDARY)
+    alignment = STACK_BOUNDARY;
+  return alignment;
+}
+
 /* Implement TARGET_FUNCTION_ARG_BOUNDARY.  Every parameter gets at
    least PARM_BOUNDARY bits of alignment, but will be given anything up
    to STACK_BOUNDARY bits if the type requires it.  */
@@ -5644,7 +5710,10 @@ mips_function_arg_boundary (machine_mode mode, const_tree type)
 {
   unsigned int alignment;
 
-  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
+  alignment = (type
+		 ? TYPE_ALIGN (TYPE_MAIN_VARIANT (type))
+		 : GET_MODE_ALIGNMENT (mode));
+
   if (alignment < PARM_BOUNDARY)
     alignment = PARM_BOUNDARY;
   if (alignment > STACK_BOUNDARY)
@@ -20108,6 +20177,8 @@ mips_promote_function_mode (const_tree type ATTRIBUTE_UNUSED,
 #define TARGET_ARG_PARTIAL_BYTES mips_arg_partial_bytes
 #undef TARGET_FUNCTION_ARG
 #define TARGET_FUNCTION_ARG mips_function_arg
+#undef TARGET_FUNCTION_INCOMING_ARG
+#define TARGET_FUNCTION_INCOMING_ARG mips_function_incoming_arg
 #undef TARGET_FUNCTION_ARG_ADVANCE
 #define TARGET_FUNCTION_ARG_ADVANCE mips_function_arg_advance
 #undef TARGET_FUNCTION_ARG_BOUNDARY
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index 803ab98..7b01f07 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -2460,6 +2460,10 @@ typedef struct mips_args {
 
   /* True if the function has a prototype.  */
   int prototype;
+
+  /* True if we have warned about possible change in how aligned arguments
+     are passed to functions.  */
+  int aligned_arg_warning_given;
 } CUMULATIVE_ARGS;
 
 /* Initialize a variable CUM of type CUMULATIVE_ARGS
diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt
index ebd67e4..1501dbd 100644
--- a/gcc/config/mips/mips.opt
+++ b/gcc/config/mips/mips.opt
@@ -396,6 +396,10 @@ mvirt
 Target Report Var(TARGET_VIRT)
 Use Virtualization Application Specific instructions.
 
+mwarn-aligned-args
+Target Report Var(mips_warn_aligned_args) Init(1)
+Warn if an aligned argument is being passed in a manner incompatible with older GCC versions.
+
 mxpa
 Target Report Var(TARGET_XPA)
 Use eXtended Physical Address (XPA) instructions.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 490df93..4f4a076 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -874,7 +874,8 @@ Objective-C and Objective-C++ Dialects}.
 -mfp-exceptions -mno-fp-exceptions @gol
 -mvr4130-align -mno-vr4130-align -msynci -mno-synci @gol
 -mrelax-pic-calls -mno-relax-pic-calls -mmcount-ra-address @gol
--mframe-header-opt -mno-frame-header-opt}
+-mframe-header-opt -mno-frame-header-opt @gol
+-mwarn-aligned-args -mno-warn-aligned-args}
 
 @emph{MMIX Options}
 @gccoptlist{-mlibfuncs  -mno-libfuncs  -mepsilon  -mno-epsilon  -mabi=gnu @gol



2016-02-24  Steve Ellcey  <sellcey@imgtec.com>

	PR target/68273
	* gcc.c-torture/execute/pr68273-1.c: New test.
	* gcc.c-torture/execute/pr68273-2.c: New test.
	* gcc.c-torture/execute/pr68273-3.c: New test.
	* gcc.target/mips/pr68273-warn.c: New test. 
	* gcc.dg/pr48335-2.c: Turn off warning on MIPS.
	* gcc.dg/torture/pr48493.c: Ditto.

diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
index e69de29..e8ac9f1 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
@@ -0,0 +1,75 @@
+/* { dg-options "-mno-warn-aligned-args" { target mips*-*-* } } */
+
+/* Make sure that the alignment attribute on an argument passed by
+   value does not affect the calling convention and what registers
+   arguments are passed in.  */
+
+extern void exit (int);
+extern void abort (void);
+
+typedef int alignedint __attribute__((aligned(8)));
+
+int  __attribute__((noinline))
+foo1 (int a, alignedint b)
+{ return a + b; }
+
+int __attribute__((noinline))
+foo2 (int a, int b)
+{
+  return a + b;
+}
+
+int __attribute__((noinline))
+bar1 (alignedint x)
+{
+  return foo1 (1, x);
+}
+
+int __attribute__((noinline))
+bar2 (alignedint x)
+{
+  return foo1 (1, (alignedint) 99);
+}
+
+int __attribute__((noinline))
+bar3 (alignedint x)
+{
+  return foo1 (1, x + (alignedint) 1);
+}
+
+alignedint q = 77;
+
+int __attribute__((noinline))
+bar4 (alignedint x)
+{
+  return foo1 (1, q);
+}
+
+int __attribute__((noinline))
+bar5 (alignedint x)
+{
+  return foo2 (1, x);
+}
+
+int __attribute__((noinline))
+use_arg_regs (int i, int j, int k)
+{
+  return i + j - k;
+}
+
+int main()
+{
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (foo1 (19,13) != 32) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar1 (-33) != -32) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar2 (1) != 100) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar3 (17) != 19) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar4 (-33) != 78) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar5 (-84) != -83) abort ();
+   exit (0);
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c b/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
index e69de29..42bec8b 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
@@ -0,0 +1,111 @@
+/* { dg-options "-mno-warn-aligned-args" { target mips*-*-* } } */
+
+/* Make sure that the alignment attribute on an argument passed by
+   value does not affect the calling convention and what registers
+   arguments are passed in.  */
+
+extern void exit (int);
+extern void abort (void);
+
+typedef struct s {
+	char c;
+	char d;
+} t1;
+typedef t1 t1_aligned8  __attribute__((aligned(8)));
+typedef t1 t1_aligned16 __attribute__((aligned(16)));
+typedef t1 t1_aligned32 __attribute__((aligned(32)));
+
+int bar1 (int a, t1 b)
+{
+	return a + b.c + b.d;
+}
+
+int bar2 (int a, t1_aligned8 b)
+{
+	return a + b.c + b.d;
+}
+
+int bar3 (int a, t1_aligned16 b)
+{
+	return a + b.c + b.d;
+}
+
+int bar4 (int a, t1_aligned32 b)
+{
+	return a + b.c + b.d;
+}
+
+#define FOODEF(n,m,type) \
+int __attribute__((noinline)) \
+foo##n (int i, type b) \
+  { \
+    return bar##m (i, b); \
+  }
+
+FOODEF(1,  1, t1)
+FOODEF(2,  1, t1_aligned8)
+FOODEF(3,  1, t1_aligned16)
+FOODEF(4,  1, t1_aligned32)
+FOODEF(5,  2, t1)
+FOODEF(6,  2, t1_aligned8)
+FOODEF(7,  2, t1_aligned16)
+FOODEF(8,  2, t1_aligned32)
+FOODEF(9,  3, t1)
+FOODEF(10, 3, t1_aligned8)
+FOODEF(11, 3, t1_aligned16)
+FOODEF(12, 3, t1_aligned32)
+FOODEF(13, 4, t1)
+FOODEF(14, 4, t1_aligned8)
+FOODEF(15, 4, t1_aligned16)
+FOODEF(16, 4, t1_aligned32)
+
+int __attribute__((noinline))
+use_arg_regs (int i, int j, int k)
+{
+  return i + j - k;
+}
+
+#define FOOCALL(i) \
+  c = i*11 + 39; \
+  x1.c = i + 5; \
+  x1.d = i*2 + 3; \
+  x2.c = x1.c + 1; \
+  x2.d = x1.d + 1; \
+  x3.c = x2.c + 1; \
+  x3.d = x2.d + 1; \
+  x4.c = x3.c + 1; \
+  x4.d = x3.d + 1; \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x1) != c + x1.c + x1.d) abort (); \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x2) != c + x2.c + x2.d) abort (); \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x3) != c + x3.c + x3.d) abort (); \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x4) != c + x4.c + x4.d) abort ();
+
+int main()
+{
+  int c;
+  t1 x1;
+  t1_aligned8 x2;
+  t1_aligned16 x3;
+  t1_aligned32 x4;
+  FOOCALL(1);
+  FOOCALL(2);
+  FOOCALL(3);
+  FOOCALL(4);
+  FOOCALL(5);
+  FOOCALL(6);
+  FOOCALL(7);
+  FOOCALL(8);
+  FOOCALL(9);
+  FOOCALL(10);
+  FOOCALL(11);
+  FOOCALL(12);
+  FOOCALL(13);
+  FOOCALL(14);
+  FOOCALL(15);
+  FOOCALL(16);
+  exit (0);
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68273-3.c b/gcc/testsuite/gcc.c-torture/execute/pr68273-3.c
index e69de29..b845159 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-3.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-3.c
@@ -0,0 +1,75 @@
+/* { dg-options "-mno-warn-aligned-args" { target mips*-*-* } } */
+
+/* Make sure that the alignment attribute on an argument passed by
+   value does not affect the calling convention and what registers
+   arguments are passed in.  */
+
+extern void exit (int);
+extern void abort (void);
+
+typedef long long alignedll __attribute__((aligned(1)));
+
+long long __attribute__((noinline))
+foo1 (int a, alignedll b)
+{ return a + b; }
+
+long long __attribute__((noinline))
+foo2 (int a, long long b)
+{
+  return a + b;
+}
+
+long long __attribute__((noinline))
+bar1 (alignedll x)
+{
+  return foo1 (1, x);
+}
+
+long long __attribute__((noinline))
+bar2 (alignedll x)
+{
+  return foo1 (1, (alignedll) 99);
+}
+
+long long __attribute__((noinline))
+bar3 (alignedll x)
+{
+  return foo1 (1, x + (alignedll) 1);
+}
+
+alignedll q = 77;
+
+long long __attribute__((noinline))
+bar4 (alignedll x)
+{
+  return foo1 (1, q);
+}
+
+long long __attribute__((noinline))
+bar5 (alignedll x)
+{
+  return foo2 (1, x);
+}
+
+long long __attribute__((noinline))
+use_arg_regs (int i, int j, int k)
+{
+  return i + j - k;
+}
+
+int main()
+{
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (foo1 (19,13) != 32) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar1 (-33) != -32) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar2 (1) != 100) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar3 (17) != 19) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar4 (-33) != 78) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar5 (-84) != -83) abort ();
+   exit (0);
+}
diff --git a/gcc/testsuite/gcc.dg/pr48335-2.c b/gcc/testsuite/gcc.dg/pr48335-2.c
index a37c079..f56b6fd 100644
--- a/gcc/testsuite/gcc.dg/pr48335-2.c
+++ b/gcc/testsuite/gcc.dg/pr48335-2.c
@@ -1,6 +1,7 @@
 /* PR middle-end/48335 */
 /* { dg-do compile } */
 /* { dg-options "-O2 -fno-tree-sra" } */
+/* { dg-options "-O2 -fno-tree-sra -mno-warn-aligned-args" { target mips*-*-* } } */
 
 typedef long long T __attribute__((may_alias, aligned (1)));
 typedef short U __attribute__((may_alias, aligned (1)));
diff --git a/gcc/testsuite/gcc.dg/torture/pr48493.c b/gcc/testsuite/gcc.dg/torture/pr48493.c
index ddb61f2..77f30c7 100644
--- a/gcc/testsuite/gcc.dg/torture/pr48493.c
+++ b/gcc/testsuite/gcc.dg/torture/pr48493.c
@@ -1,4 +1,5 @@
 /* { dg-do compile } */
+/* { dg-options "-mno-warn-aligned-args" { target mips*-*-* } } */
 
 typedef long long T __attribute__((may_alias, aligned (1)));
 
diff --git a/gcc/testsuite/gcc.target/mips/pr68273-warn.c b/gcc/testsuite/gcc.target/mips/pr68273-warn.c
index e69de29..27ef1b1 100644
--- a/gcc/testsuite/gcc.target/mips/pr68273-warn.c
+++ b/gcc/testsuite/gcc.target/mips/pr68273-warn.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-skip-if "needs asm output" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
+
+typedef int alignedint __attribute__((aligned(8)));
+extern void foo1 (alignedint y);
+extern void foo2 (int y);
+extern void foo3 (alignedint y);
+extern void foo4 (int y);
+
+int
+splat1 (alignedint b) /* { dg-warning "incompatible with previous GCC versions" } */
+{
+  foo1 (b); /* { dg-warning "incompatible with previous GCC versions" } */
+}
+
+int
+splat2 (alignedint b) /* { dg-warning "incompatible with previous GCC versions" } */
+{
+  foo2 (b); /* { dg-warning "incompatible with previous GCC versions" } */
+}
+
+int
+splat3 (int b)
+{
+  foo3 (b);
+}
+
+int
+splat4 (int b)
+{
+  foo4 (b);
+}