[Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Steve Ellcey]

This is a patch for PR 68273, where MIPS is passing arguments in the wrong
registers.  The problem is that when passing an argument by value,
mips_function_arg_boundary was looking at the type of the argument (and
not just the mode), and if that argument was a variable with extra alignment
info (say an integer variable with 8 byte alignment), MIPS was aligning the
argument on an 8 byte boundary instead of a 4 byte boundary the way it should.

Since we are passing values (not variables), the alignment of the variable
that the value is copied from should not affect the alignment of the value
being passed.

This patch fixes the problem and it could change what registers arguments
are passed in, which means it could affect backwards compatibility with
older programs.  But since the current behaviour is not compliant with the
MIPS ABI and does not match what LLVM does, I think we have to make this
change.  For the most part this will only affect arguments which are copied
from variables that have non-standard alignments set by the aligned attribute,
however the SRA optimization pass can create aligned variables as it splits
aggregates apart and that was what triggered this bug report.

This is basically the same bug as the ARM bug PR 65956 and the fix is
pretty much the same too.

Rather than create MIPS specific tests that check the use of specific
registers I created two tests to put in gcc.c-torture/execute that
were failing before because GCC on MIPS was not consistent in where
arguments were passed and which now work with this patch.

Tested with mips-mti-linux-gnu and no regressions.  OK to checkin?

Steve Ellcey
sellcey@imgtec.com


2016-01-29  Steve Ellcey  <sellcey@imgtec.com>

	PR target/68273
	* config/mips/mips.c (mips_function_arg_boundary): Fix argument
	alignment.



diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index dd54d6a..ecce3cd 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -5643,8 +5643,9 @@ static unsigned int
 mips_function_arg_boundary (machine_mode mode, const_tree type)
 {
   unsigned int alignment;
-
-  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
+  alignment = type && mode == BLKmode
+	      ? TYPE_ALIGN (TYPE_MAIN_VARIANT (type))
+	      : GET_MODE_ALIGNMENT (mode);
   if (alignment < PARM_BOUNDARY)
     alignment = PARM_BOUNDARY;
   if (alignment > STACK_BOUNDARY)


2016-01-29  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.


diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
index e69de29..3ce07c6 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
@@ -0,0 +1,74 @@
+/* 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..1661be9 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
@@ -0,0 +1,109 @@
+/* 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);
+}

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Richard Sandiford]

I'm not sure this patch is safe.  The received wisdom used to be that
ABIs should be defined in terms of types, not modes, since types
represent the source code while modes are an internal GCC concept
that could change over time (although in practice the barrier for
that is pretty high).

The patch that introduced the line being patched was part of a series
that fixed ABI incompatibilities with MIPSpro, and IIRC some of the
incompatibilties really were due to us looking at modes when we should
have been looking at types.

So...

"Steve Ellcey " <sellcey@imgtec.com> writes:
> This is a patch for PR 68273, where MIPS is passing arguments in the wrong
> registers.  The problem is that when passing an argument by value,
> mips_function_arg_boundary was looking at the type of the argument (and
> not just the mode), and if that argument was a variable with extra alignment
> info (say an integer variable with 8 byte alignment), MIPS was aligning the
> argument on an 8 byte boundary instead of a 4 byte boundary the way it should.
>
> Since we are passing values (not variables), the alignment of the variable
> that the value is copied from should not affect the alignment of the value
> being passed.

...to me this suggests we might have a middle-end bug.  The argument
seems to be that the alignment of the type being passed in cannot
reasonably be used to determine the ABI for semantic reasons
(rvalues don't have meaningful alignment).  Doesn't that mean that
we're passing the wrong type to the hook?  The point of the hook
is to say how an ABI handles a particular type, so if we have a type
variant that the ABI can't reasonably handle directly for language
reasons, shouldn't we be passing the main variant instead?

> This patch fixes the problem and it could change what registers arguments
> are passed in, which means it could affect backwards compatibility with
> older programs.  But since the current behaviour is not compliant with the
> MIPS ABI and does not match what LLVM does, I think we have to make this
> change.  For the most part this will only affect arguments which are copied
> from variables that have non-standard alignments set by the aligned attribute,
> however the SRA optimization pass can create aligned variables as it splits
> aggregates apart and that was what triggered this bug report.
>
> This is basically the same bug as the ARM bug PR 65956 and the fix is
> pretty much the same too.
>
> Rather than create MIPS specific tests that check the use of specific
> registers I created two tests to put in gcc.c-torture/execute that
> were failing before because GCC on MIPS was not consistent in where
> arguments were passed and which now work with this patch.
>
> Tested with mips-mti-linux-gnu and no regressions.  OK to checkin?

Given the argument about LLVM, I think it would also be worth running
the compat testsuite with clang as the alt compiler both before and
after the patch to see whether we regress.

> 2016-01-29  Steve Ellcey  <sellcey@imgtec.com>
>
> 	PR target/68273
> 	* config/mips/mips.c (mips_function_arg_boundary): Fix argument
> 	alignment.
>
>
>
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index dd54d6a..ecce3cd 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -5643,8 +5643,9 @@ static unsigned int
>  mips_function_arg_boundary (machine_mode mode, const_tree type)
>  {
>    unsigned int alignment;
> -
> -  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
> +  alignment = type && mode == BLKmode
> +	      ? TYPE_ALIGN (TYPE_MAIN_VARIANT (type))
> +	      : GET_MODE_ALIGNMENT (mode);
>    if (alignment < PARM_BOUNDARY)
>      alignment = PARM_BOUNDARY;
>    if (alignment > STACK_BOUNDARY)

We need to be careful of examples like:

  struct __attribute__ ((aligned (8))) s { _Complex float x; };
  void foo (struct s *ptr, struct s val) { *ptr = val; }

"x" gets SCmode, which has an alignment of 4.  And it's OK for TYPE_MODE
to have a smaller alignment than the type -- it's just not allowed to
have a larger alignment (and even that restriction only applies because
this is a STRICT_ALIGNMENT target).  So the structure itself inherits
this SCmode.

The patch therefore changes how we handle foo() for -mabi=32 -msoft-float.
Before the patch "val" is passed in $6 and $7.  After the patch it's
passed in $5 and $6.  clang behaves like the unpatched GCC.

If instead we use:

  struct __attribute__ ((aligned (8))) s { float x; float y; };
  void foo (struct s *ptr, struct s val) { *ptr = val; }

then the structure has BLKmode and the alignment is honoured both before
and after the patch.

There's no real ABI reason for handling the two cases differently.
The fact that one gets BLKmode and the other doesn't is down
to GCC internals.

We also have to be careful about the STRICT_ALIGNMENT thing.
At the moment that's hard-coded to 1 for MIPS, but it's possible that
it could become configurable in future, like it is for aarch64 and
rs6000.  !STRICT_ALIGNMENT allows TYPE_MODE to have a larger
alignment than the type, so underaligned structures would get modes
when they didn't previously.  That would in turn change how they're
passed as arguments.

Thanks,
Richard

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Richard Biener]

On Sat, Jan 30, 2016 at 12:06 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> I'm not sure this patch is safe.  The received wisdom used to be that
> ABIs should be defined in terms of types, not modes, since types
> represent the source code while modes are an internal GCC concept
> that could change over time (although in practice the barrier for
> that is pretty high).
>
> The patch that introduced the line being patched was part of a series
> that fixed ABI incompatibilities with MIPSpro, and IIRC some of the
> incompatibilties really were due to us looking at modes when we should
> have been looking at types.
>
> So...
>
> "Steve Ellcey " <sellcey@imgtec.com> writes:
>> This is a patch for PR 68273, where MIPS is passing arguments in the wrong
>> registers.  The problem is that when passing an argument by value,
>> mips_function_arg_boundary was looking at the type of the argument (and
>> not just the mode), and if that argument was a variable with extra alignment
>> info (say an integer variable with 8 byte alignment), MIPS was aligning the
>> argument on an 8 byte boundary instead of a 4 byte boundary the way it should.
>>
>> Since we are passing values (not variables), the alignment of the variable
>> that the value is copied from should not affect the alignment of the value
>> being passed.
>
> ...to me this suggests we might have a middle-end bug.  The argument
> seems to be that the alignment of the type being passed in cannot
> reasonably be used to determine the ABI for semantic reasons
> (rvalues don't have meaningful alignment).  Doesn't that mean that
> we're passing the wrong type to the hook?  The point of the hook
> is to say how an ABI handles a particular type, so if we have a type
> variant that the ABI can't reasonably handle directly for language
> reasons, shouldn't we be passing the main variant instead?

Actually I'd say it's a frontend bug then setting DECL_ARG_TYPE to the
wrong type.  Quoting:

/* For a PARM_DECL, records the data type used to pass the argument,
   which may be different from the type seen in the program.  */
#define DECL_ARG_TYPE(NODE) (PARM_DECL_CHECK (NODE)->decl_common.initial)

if the type doesn't end up coming from DECL_ARG_TYPE but the value
then I'd agree - we should always look at the main variant here.  Expansion is
quite twisty here so catching all cases is going to be interesting as well as
evaluating ABI side-effects - that's much easier to determine when you change
one target at a time...

Richard.

>> This patch fixes the problem and it could change what registers arguments
>> are passed in, which means it could affect backwards compatibility with
>> older programs.  But since the current behaviour is not compliant with the
>> MIPS ABI and does not match what LLVM does, I think we have to make this
>> change.  For the most part this will only affect arguments which are copied
>> from variables that have non-standard alignments set by the aligned attribute,
>> however the SRA optimization pass can create aligned variables as it splits
>> aggregates apart and that was what triggered this bug report.
>>
>> This is basically the same bug as the ARM bug PR 65956 and the fix is
>> pretty much the same too.
>>
>> Rather than create MIPS specific tests that check the use of specific
>> registers I created two tests to put in gcc.c-torture/execute that
>> were failing before because GCC on MIPS was not consistent in where
>> arguments were passed and which now work with this patch.
>>
>> Tested with mips-mti-linux-gnu and no regressions.  OK to checkin?
>
> Given the argument about LLVM, I think it would also be worth running
> the compat testsuite with clang as the alt compiler both before and
> after the patch to see whether we regress.
>
>> 2016-01-29  Steve Ellcey  <sellcey@imgtec.com>
>>
>>       PR target/68273
>>       * config/mips/mips.c (mips_function_arg_boundary): Fix argument
>>       alignment.
>>
>>
>>
>> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
>> index dd54d6a..ecce3cd 100644
>> --- a/gcc/config/mips/mips.c
>> +++ b/gcc/config/mips/mips.c
>> @@ -5643,8 +5643,9 @@ static unsigned int
>>  mips_function_arg_boundary (machine_mode mode, const_tree type)
>>  {
>>    unsigned int alignment;
>> -
>> -  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
>> +  alignment = type && mode == BLKmode
>> +           ? TYPE_ALIGN (TYPE_MAIN_VARIANT (type))
>> +           : GET_MODE_ALIGNMENT (mode);
>>    if (alignment < PARM_BOUNDARY)
>>      alignment = PARM_BOUNDARY;
>>    if (alignment > STACK_BOUNDARY)
>
> We need to be careful of examples like:
>
>   struct __attribute__ ((aligned (8))) s { _Complex float x; };
>   void foo (struct s *ptr, struct s val) { *ptr = val; }
>
> "x" gets SCmode, which has an alignment of 4.  And it's OK for TYPE_MODE
> to have a smaller alignment than the type -- it's just not allowed to
> have a larger alignment (and even that restriction only applies because
> this is a STRICT_ALIGNMENT target).  So the structure itself inherits
> this SCmode.
>
> The patch therefore changes how we handle foo() for -mabi=32 -msoft-float.
> Before the patch "val" is passed in $6 and $7.  After the patch it's
> passed in $5 and $6.  clang behaves like the unpatched GCC.
>
> If instead we use:
>
>   struct __attribute__ ((aligned (8))) s { float x; float y; };
>   void foo (struct s *ptr, struct s val) { *ptr = val; }
>
> then the structure has BLKmode and the alignment is honoured both before
> and after the patch.
>
> There's no real ABI reason for handling the two cases differently.
> The fact that one gets BLKmode and the other doesn't is down
> to GCC internals.
>
> We also have to be careful about the STRICT_ALIGNMENT thing.
> At the moment that's hard-coded to 1 for MIPS, but it's possible that
> it could become configurable in future, like it is for aarch64 and
> rs6000.  !STRICT_ALIGNMENT allows TYPE_MODE to have a larger
> alignment than the type, so underaligned structures would get modes
> when they didn't previously.  That would in turn change how they're
> passed as arguments.
>
> Thanks,
> Richard

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Eric Botcazou]

> Actually I'd say it's a frontend bug then setting DECL_ARG_TYPE to the
> wrong type.  Quoting:
> 
> /* For a PARM_DECL, records the data type used to pass the argument,
>    which may be different from the type seen in the program.  */
> #define DECL_ARG_TYPE(NODE) (PARM_DECL_CHECK (NODE)->decl_common.initial)
> 
> if the type doesn't end up coming from DECL_ARG_TYPE but the value
> then I'd agree - we should always look at the main variant here.  Expansion
> is quite twisty here so catching all cases is going to be interesting as
> well as evaluating ABI side-effects - that's much easier to determine when
> you change one target at a time...

FWIW SPARC 64-bit is very likely affected too, but I'm not sure we would want 
to change the ABI (it's a GCC extension after all).  IMO the underlying issue 
is to allow variants with different alignments (for strict-alignment targets 
at least), this seems fundamentally broken to me.

-- 
Eric Botcazou

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Steve Ellcey]

On Sat, 2016-01-30 at 11:06 +0000, Richard Sandiford wrote:
> I'm not sure this patch is safe.  The received wisdom used to be that
> ABIs should be defined in terms of types, not modes, since types
> represent the source code while modes are an internal GCC concept
> that could change over time (although in practice the barrier for
> that is pretty high).
> 
> The patch that introduced the line being patched was part of a series
> that fixed ABI incompatibilities with MIPSpro, and IIRC some of the
> incompatibilties really were due to us looking at modes when we should
> have been looking at types.

My main reason for looking at mode instead of type for non-aggregates
was because of argument promotion.  When I looked at a function with a
char argument, the type was char but the mode was SI.  Now I could still
use the alignment of char (1 byte) since it would get extended to 4
bytes in mips_function_arg_boundary by:

  if (alignment < PARM_BOUNDARY)
    alignment = PARM_BOUNDARY;

But it still seemed a bit 'wrong' to me.  Maybe something in the
front/middle ends should be updating the type to match any argument
promotion that is being done?

Steve Ellcey
sellcey@imgtec.com

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Steve Ellcey]

On Sat, 2016-01-30 at 11:06 +0000, Richard Sandiford wrote:

> We need to be careful of examples like:
> 
>   struct __attribute__ ((aligned (8))) s { _Complex float x; };
>   void foo (struct s *ptr, struct s val) { *ptr = val; }
> 
> "x" gets SCmode, which has an alignment of 4.  And it's OK for TYPE_MODE
> to have a smaller alignment than the type -- it's just not allowed to
> have a larger alignment (and even that restriction only applies because
> this is a STRICT_ALIGNMENT target).  So the structure itself inherits
> this SCmode.
> 
> The patch therefore changes how we handle foo() for -mabi=32 -msoft-float.
> Before the patch "val" is passed in $6 and $7.  After the patch it's
> passed in $5 and $6.  clang behaves like the unpatched GCC.
> 
> If instead we use:
> 
>   struct __attribute__ ((aligned (8))) s { float x; float y; };
>   void foo (struct s *ptr, struct s val) { *ptr = val; }
> 
> then the structure has BLKmode and the alignment is honoured both before
> and after the patch.
> 
> There's no real ABI reason for handling the two cases differently.
> The fact that one gets BLKmode and the other doesn't is down
> to GCC internals.
> 
> We also have to be careful about the STRICT_ALIGNMENT thing.
> At the moment that's hard-coded to 1 for MIPS, but it's possible that
> it could become configurable in future, like it is for aarch64 and
> rs6000.  !STRICT_ALIGNMENT allows TYPE_MODE to have a larger
> alignment than the type, so underaligned structures would get modes
> when they didn't previously.  That would in turn change how they're
> passed as arguments.
> 
> Thanks,
> Richard

Richard,

Can you explain why the GCC internals cause us to get SCmode instead of
BLKmode for the example with _Complex?  I don't understand that.  It
seems wrong to me and I don't understand where it is coming from.

Steve Ellcey
sellcey@imgtec.com

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Eric Botcazou]

> Can you explain why the GCC internals cause us to get SCmode instead of
> BLKmode for the example with _Complex?  I don't understand that.  It
> seems wrong to me and I don't understand where it is coming from.

compute_record_mode uses BLKmode only as a last resort because this will 
significantly pessimize over non-BLKmode at the RTL level.  As Richard said 
(and as discussed many times over the years), you ought not to rely on the 
mode for the calling convention of aggregate types.

-- 
Eric Botcazou

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Steve Ellcey]

Here is a new patch for PR target/68273.  It makes the GCC calling conventions
compatible with LLVM so that the two agree with each other in all the cases
I could think of testing and it fixes the reported defect.

I couldn't get the GCC compatibility test to work (see
https://gcc.gnu.org/ml/gcc/2016-02/msg00017.html) so I wasn't able to use
that for compatibility testing, instead I hand examined routines with various
argument types (structures, ints, complex, etc) to see what registers 
GCC and LLVM were accessing.

This change does introduce an ABI/compatibility change with GCC itself and
that is obviously a concern.  Basically, any type that is passed by value
and has an external alignment applied to it may get passed in different
registers because we strip off that alignment (via TYPE_MAIN_VARIANT) before
determining the alignment of the variable.

If we don't want to break the GCC compatibility then we will continue to have an
incompatibility with LLVM and we will need to find another way to deal
with the aligned int variable that SRA is creating and passing to a function
that expects a 'normal' integer.

One thought I had was that we could compare TYPE_ALIGN (type) and
TYPE_ALIGN (TYPE_MAIN_VARIANT (type) and if they are different, issue
a warning during compilation about a possible incompatibility with
older objects.

Steve Ellcey
sellcey@imgtec.com


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

	PR target/68273
	* config/mips/mips.c (mips_function_arg_boundary): Fix argument
	alignment.


diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 697abc2..4aa215f 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -5644,7 +5644,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)




2016-02-03  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.


diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
index e69de29..3ce07c6 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
@@ -0,0 +1,74 @@
+/* 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..1661be9 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
@@ -0,0 +1,109 @@
+/* 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);
+}

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Mike Stump]

On Feb 3, 2016, at 2:30 PM, Steve Ellcey <sellcey@imgtec.com> wrote:
> Here is a new patch for PR target/68273.

So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc, tilegx, tilepro or xtensa.

:-(  That’s one of the problems by having each port copy and paste swaths of code from other ports to express the same thing instead of ports sharing just one copy of code.  My port is also broken in the same way (currently).

I’m curious why the caller of the hook can’t grab the main variant, if it wants.  If it did this, then all the ports are fixed wrt this issue.

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Eric Botcazou]

> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
> tilegx, tilepro or xtensa.
> :-(  That’s one of the problems by having each port copy and paste swaths of
> :code from other ports to express the same thing instead of ports sharing
> :just one copy of code.  My port is also broken in the same way
> :(currently).

Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of 
the compatibility with LLVM has IMO little merit since it's a GCC extension.

> I’m curious why the caller of the hook can’t grab the main variant, if it
> wants.  If it did this, then all the ports are fixed wrt this issue.

Or just fix PRE wrt the alignment discrepancy, which looks a lot safer to me.
It's not because this works on x86 that this is necessarily correct for all 
the other architectures, especially the strict-alignment architectures.

-- 
Eric Botcazou

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Richard Biener]

On Thu, Feb 4, 2016 at 12:09 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
>> tilegx, tilepro or xtensa.
>> :-(  That’s one of the problems by having each port copy and paste swaths of
>> :code from other ports to express the same thing instead of ports sharing
>> :just one copy of code.  My port is also broken in the same way
>> :(currently).
>
> Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of
> the compatibility with LLVM has IMO little merit since it's a GCC extension.

True, but the compiler bug is in the backends - it just now gets
exposed more easily
(read: w/o user intervention).

>> I’m curious why the caller of the hook can’t grab the main variant, if it
>> wants.  If it did this, then all the ports are fixed wrt this issue.
>
> Or just fix PRE wrt the alignment discrepancy, which looks a lot safer to me.
> It's not because this works on x86 that this is necessarily correct for all
> the other architectures, especially the strict-alignment architectures.

Doesn't "fix" the same issue popping up in other passes.

Richard.

> --
> Eric Botcazou

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Richard Biener]

On Thu, Feb 4, 2016 at 1:49 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Feb 4, 2016 at 12:09 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
>>> tilegx, tilepro or xtensa.
>>> :-(  That’s one of the problems by having each port copy and paste swaths of
>>> :code from other ports to express the same thing instead of ports sharing
>>> :just one copy of code.  My port is also broken in the same way
>>> :(currently).
>>
>> Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of
>> the compatibility with LLVM has IMO little merit since it's a GCC extension.
>
> True, but the compiler bug is in the backends - it just now gets
> exposed more easily
> (read: w/o user intervention).
>
>>> I’m curious why the caller of the hook can’t grab the main variant, if it
>>> wants.  If it did this, then all the ports are fixed wrt this issue.
>>
>> Or just fix PRE wrt the alignment discrepancy, which looks a lot safer to me.
>> It's not because this works on x86 that this is necessarily correct for all
>> the other architectures, especially the strict-alignment architectures.
>
> Doesn't "fix" the same issue popping up in other passes.

Btw, one think I noticed is passes creating tempoaries with qualified type
which is not necessary.  Not knowing enough about the PRE issue, does

Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c (revision 233136)
+++ gcc/tree-ssanames.c (working copy)
@@ -286,7 +286,7 @@ make_ssa_name_fn (struct function *fn, t

   if (TYPE_P (var))
     {
-      TREE_TYPE (t) = var;
+      TREE_TYPE (t) = TYPE_MAIN_VARIANT (var);
       SET_SSA_NAME_VAR_OR_IDENTIFIER (t, NULL_TREE);
     }
   else

fix it?  I played with sth similar for create_tmp_{reg,var} in the
past but don't
remember why I dropped the ball on it.  At least changing create_tmp_reg
would be obvious (though that misses an assert the passed in type is
a gimple register type).  For create_tmp_var it might not be that easy.

Richard.

> Richard.
>
>> --
>> Eric Botcazou

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Steve Ellcey]

On Thu, 2016-02-04 at 12:09 +0100, Eric Botcazou wrote:
> > So this doesnĆæt fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
> > tilegx, tilepro or xtensa.
> > :-(  ThatĆæs one of the problems by having each port copy and paste swaths of
> > :code from other ports to express the same thing instead of ports sharing
> > :just one copy of code.  My port is also broken in the same way
> > :(currently).
> 
> Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of 
> the compatibility with LLVM has IMO little merit since it's a GCC extension.

But it is a GCC extension that is implemented in LLVM.  It's just
implemented differently there.  We either have to change GCC, change
LLVM, or live with the difference.

In any of these cases I wonder if we should change GCC so that the code
below generates warnings.  If the two types are going to get passed
differently we shouldn't allow calls with one type to call a function
expecting the other type to happen with no warning or error.  Currently
this code does not warn even with -Wall.


typedef int alignedint __attribute__((aligned(8)));
extern void foo (alignedint a);
extern void bar (int);

int a;
alignedint b;

int main()
{
        foo(a);
        bar(b);
}


Steve Ellcey
sellcey@imgtec.com

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Richard Sandiford]

Richard Biener <richard.guenther@gmail.com> writes:
> On Thu, Feb 4, 2016 at 12:09 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
>>> tilegx, tilepro or xtensa.
>>> :-(  That’s one of the problems by having each port copy and paste swaths of
>>> :code from other ports to express the same thing instead of ports sharing
>>> :just one copy of code.  My port is also broken in the same way
>>> :(currently).
>>
>> Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of
>> the compatibility with LLVM has IMO little merit since it's a GCC extension.
>
> True, but the compiler bug is in the backends - it just now gets
> exposed more easily
> (read: w/o user intervention).

I'm still not convinced it's a backend/target bug though.  It seems
like a similar situation to floats being promoted to doubles and
chars to ints when passed to unprototyped functions or varargs.
In those situations it's up to the hook to decide what type the
value actually gets passed as.  Passing an overaligned int as a
plain int is pretty similar and isn't something that each target
should need to check individually.

Thanks,
Richard

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Richard Biener]

On February 4, 2016 9:28:22 PM GMT+01:00, Richard Sandiford <rdsandiford@googlemail.com> wrote:
>Richard Biener <richard.guenther@gmail.com> writes:
>> On Thu, Feb 4, 2016 at 12:09 PM, Eric Botcazou
><ebotcazou@adacore.com> wrote:
>>>> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000,
>rx, sparc,
>>>> tilegx, tilepro or xtensa.
>>>> :-(  That’s one of the problems by having each port copy and paste
>swaths of
>>>> :code from other ports to express the same thing instead of ports
>sharing
>>>> :just one copy of code.  My port is also broken in the same way
>>>> :(currently).
>>>
>>> Yes, fixing a compiler bug by changing the ABI is a no-no, and the
>argument of
>>> the compatibility with LLVM has IMO little merit since it's a GCC
>extension.
>>
>> True, but the compiler bug is in the backends - it just now gets
>> exposed more easily
>> (read: w/o user intervention).
>
>I'm still not convinced it's a backend/target bug though.  It seems
>like a similar situation to floats being promoted to doubles and
>chars to ints when passed to unprototyped functions or varargs.
>In those situations it's up to the hook to decide what type the
>value actually gets passed as.  Passing an overaligned int as a
>plain int is pretty similar and isn't something that each target
>should need to check individually.

I agree here.  BUT we are still effectively changing the ABI of each affected target then without actually knowing we do.
I'd happily do such change in the middle-end if we identified all affected targets and verified the changes impact.

That's a lot of work and thus having active targets follow the "arm way" of fixing the issue in the target is very reasonable in my opinion and the only thing I'd accept at this stage.

Richard.

>Thanks,
>Richard

Re: [Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Eric Botcazou]

> True, but the compiler bug is in the backends - it just now gets
> exposed more easily (read: w/o user intervention).

As far as I can see there are no problematic types in the source code, i.e. 
it's plain ANSI C, so we shouldn't be creating types with non-canonical 
alignment from that.

> Doesn't "fix" the same issue popping up in other passes.

Which pass does the alignment promotion?  Maybe it's the one to be fixed.

-- 
Eric Botcazou