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] PR target/18916; Function arg passing mem align fixes.


Richard Sandiford <rsandifo@redhat.com> writes:
>> I think we want both move_block_from_reg fixed and some tweak to the
>> alignment here.  I'd suggested something based off LOCAL_ALIGNMENT,
>> but using a MAX there might be just as good.
>
> OK, I'm bootstrapping the MAX patch now.  Should have the results
> tomorrow or Saturday.
> [...]
> --- function.c	13 Jan 2005 17:06:44 -0000	1.602
> +++ function.c	20 Jan 2005 21:54:29 -0000
> @@ -2609,7 +2609,8 @@ assign_parm_setup_block (struct assign_p
>    if (stack_parm == 0)
>      {
>        stack_parm = assign_stack_local (BLKmode, size_stored,
> -				       TYPE_ALIGN (data->passed_type));
> +				       MAX (TYPE_ALIGN (data->passed_type),
> +					    BITS_PER_WORD));
>        if (GET_MODE_SIZE (GET_MODE (entry_parm)) == size)
>  	PUT_MODE (stack_parm, GET_MODE (entry_parm));
>        set_mem_attributes (stack_parm, parm, 1);

Although that works, I realised later that it wasn't technically correct.
The MEM's attributes would still be set from PARM, so MEM_ALIGN would be
DECL_ALIGN (parm), not the alignment passed to assign_stack_local.
Because move_blkmode_from_reg assumes word accesses are OK, we would wind
up with DImode MEMs that really are 64-bit aligned, but for which MEM_ALIGN
was only 32.  Nothing seems to care, but it's clearly still wrong.

FWIW, it looks like the code before Alan's patch had the same problem:

> -      if (stack_parm == 0)
> -	{
> -	  stack_parm = assign_stack_local (BLKmode, size_stored, 0);
> -	  data->stack_parm = stack_parm;
> -	  PUT_MODE (stack_parm, GET_MODE (entry_parm));
> -	  set_mem_attributes (stack_parm, parm, 1);
> -	}

The patch below increases PARM's alignment instead.  I wondered about
making this conditional on STRICT_ALIGNMENT, but since we're already
rounding up the size up to a word boundary (see the assignment to
size_rounded at the top of the patch context), there didn't really
seem much point.  We might as well take advantage of the extra
alignment everywhere.

Patch bootstrapped & regression tested on mips-sgi-irix6.5.  New test
results here:

    http://gcc.gnu.org/ml/gcc-testresults/2005-01/msg01005.html

OK to install?

Richard


	* function.c (assign_parm_setup_block): When creating a new stack slot
	for a parameter, get its alignment from the parameter's DECL_ALIGN
	rather than the type's TYPE_ALIGN.  Make sure that the parameter
	is at least word aligned.

Index: function.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/function.c,v
retrieving revision 1.602
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.602 function.c
*** function.c	13 Jan 2005 17:06:44 -0000	1.602
--- function.c	21 Jan 2005 12:02:20 -0000
*************** assign_parm_setup_block (struct assign_p
*** 2608,2615 ****
    size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
    if (stack_parm == 0)
      {
        stack_parm = assign_stack_local (BLKmode, size_stored,
! 				       TYPE_ALIGN (data->passed_type));
        if (GET_MODE_SIZE (GET_MODE (entry_parm)) == size)
  	PUT_MODE (stack_parm, GET_MODE (entry_parm));
        set_mem_attributes (stack_parm, parm, 1);
--- 2608,2616 ----
    size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
    if (stack_parm == 0)
      {
+       DECL_ALIGN (parm) = MAX (DECL_ALIGN (parm), BITS_PER_WORD);
        stack_parm = assign_stack_local (BLKmode, size_stored,
! 				       DECL_ALIGN (parm));
        if (GET_MODE_SIZE (GET_MODE (entry_parm)) == size)
  	PUT_MODE (stack_parm, GET_MODE (entry_parm));
        set_mem_attributes (stack_parm, parm, 1);


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