This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Steve Ellcey  <sellcey@imgtec.com> writes:
> Here is a new patch for PR 68273.  The original problem with gsoap has
> been fixed by changing GCC to not create overly-aligned variables in
> the SRA pass but the MIPS specific problem of how user-aligned variables
> are passed to functions remains.
> 
> Because MIPS GCC is internally inconsistent, it seems like changing
> the passing convention for user aligned variables on MIPS is the best
> option, even though this is an ABI change for MIPS.
> 
> For example:
> 
> 	typedef int alignedint __attribute__((aligned(8)));
> 	int foo1 (int x, alignedint y) { return x+y; }
> 	int foo2 (int x, int y) { return x+y; }
> 	int foo3 (int x, alignedint y) { return x+y; }
> 	int foo4 (int x, int y) { return x+y; }
> 	int splat1(int a, alignedint b) { foo1(a,b); }
> 	int splat2(int a, alignedint b) { foo2(a,b); }
> 	int splat3(int a, int b) { foo3(a,b); }
> 	int splat4(int a, int b) { foo4(a,b); }
> 
> In this case foo1 and foo3 would expect the second argument to be in
> register $6, but foo2 and foo4 would exect it in register $5.
> 
> Likewise splat1 and splat2 would pass the second argument in $6, but
> splat3 and splat4 would pass it in $5.
> 
> This means that the call from splat2 to foo2 and the call from splat3
> to foo3 would be wrong in that the caller is putting the argument in
> one register but the callee is getting out of a different register.

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.

> In none of these cases would GCC give a warning about the inconsistent
> parameter passing
> 
> Since this patch could cause a change in the users program, I have
> added a warning that will be emitted whenever a user passes an
> aligned type or when a parameter is declared as an aligned type
> and that alignment could cause a change in how the value is passed.
> 
> I also added a way to turn that warning off in case the user doesn't
> want to see it (-mno-warn-aligned-args).  I did not add an option
> to make GCC pass arguments in the old manner as I consider that method
> of passing arguments to be a bug and I don't think we want to have an
> option to propogate that incorrect behavior.

This sounds good to me.

I'd like to wait and see if anyone else has comments here given the
amount of discussion there has been on this PR.

I've pointed out a couple of style issues inline below.

Matthew

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

Needs doc/invoke.texi update.

> 	* 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.
> 
> 
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index 697abc2..05465c1 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,50 @@ 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);

Long line, split the string. Space around +

> +      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);

Long line and space around +.

> +      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 +5680,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 +5706,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);
> +

Add brackets to RHS multi-line expression

>    if (alignment < PARM_BOUNDARY)
>      alignment = PARM_BOUNDARY;
>    if (alignment > STACK_BOUNDARY)
> @@ -5652,6 +5717,7 @@ mips_function_arg_boundary (machine_mode mode,
> const_tree type)
>    return alignment;
>  }
> 
> +

Extra whitespace.

>  /* Implement TARGET_GET_RAW_RESULT_MODE and TARGET_GET_RAW_ARG_MODE.
> */
> 
>  static machine_mode
> @@ -20108,6 +20174,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..c836a23 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. */

Double space after .

> +  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..d6f486d 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 incomatible
> with older GCC versions.
> +

incompatible

>  mxpa
>  Target Report Var(TARGET_XPA)
>  Use eXtended Physical Address (XPA) instructions.
> 
> 
> 
> 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.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..7cfd654 100644
> --- a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
> @@ -0,0 +1,76 @@
> +/* { 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;

Spaces around +/-

> +}
> +
> +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..157dbfe 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)

Space after bar1 and throughout.

> +{
> +	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;

Spaces round +/-

> +}
> +
> +#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.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.

> 
>  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..f1b0dc8 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" }

Space after splat1 and foo1. Likewise for the rest of the file.

> */
> +}
> +
> +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);
> +}


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]