[PATCH] Fix unaligned load on the IA-64

[PATCH] Fix unaligned load on the IA-64

[Eric Botcazou]

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

Hi,

for the testcase:

  type R1 is record
    A1, A2, A3 : System.Address;
  end record;

  type R2 is record
    C : Character;
    R : R1;
  end record;
  pragma Pack (R2);

  procedure Dummy (R : R1) is begin null; end;

  procedure Init (X : R2) is
  begin
    Dummy (X.R);
  end;

R1 is passed in registers on the IA-64 and for Dummy (X.R) the compiler 
(move_block_to_reg) generates an unaligned word move:

(insn 9 8 10 3 q.adb:22 (set (reg:DI 120 out0)
        (mem/s/j:DI (reg/f:DI 341) [0 S8 A8])) -1 (nil))

(gdb) frame 1
#1  0x00000000007baa32 in load_register_parameters (args=0x7fffffff9c30,
    num_actuals=1, call_fusage=0x7fffffff9db8, flags=0, is_sibcall=0,
    sibcall_failure=0x7fffffff9d8c) at /home/eric/svn/gcc/gcc/calls.c:1697
1697                    move_block_to_reg (REGNO (reg), mem, nregs, 
args[i].mode);
(gdb) p debug_rtx(reg)
(reg:BLK 120 out0)
$1 = void
(gdb) p debug_rtx(mem)
(mem/s/j:BLK (reg/f:DI 341) [0 S24 A8])


The calls.c code contains a machinery that could prevent that from happening:

/* If any elements in ARGS refer to parameters that are to be passed in
   registers, but not in memory, and whose alignment does not permit a
   direct copy into registers.  Copy the values into a group of pseudos
   which we will later copy into the appropriate hard registers.

   Pseudos for each unaligned argument will be stored into the array
   args[argnum].aligned_regs.  The caller is responsible for deallocating
   the aligned_regs array if it is nonzero.  */

static void
store_unaligned_arguments_into_pseudos (struct arg_data *args, int 
num_actuals)
{
  int i, j;

  for (i = 0; i < num_actuals; i++)
    if (args[i].reg != 0 && ! args[i].pass_on_stack
        && args[i].mode == BLKmode
        && (TYPE_ALIGN (TREE_TYPE (args[i].tree_value))
            < (unsigned int) MIN (BIGGEST_ALIGNMENT, BITS_PER_WORD)))

but it doesn't kick in since the type is sufficiently aligned:

(gdb) p debug_tree(args[i]->tree_value)
    type <record_type 0x2aaaab226a80 q__r1 sizes-gimplified BLK
        size <integer_cst 0x2aaaab180b40 constant 192>
        unit size <integer_cst 0x2aaaab2399f0 constant 24>
        align 64 symtab 0 alias set -1 canonical type 0x2aaaab226a80

although the actual argument isn't:

(gdb) p debug_rtx(args[i]->value)
(mem/s/j:BLK (reg/f:DI 341) [0 S24 A8])

because of the packedness.


Fixed by adding a check on the alignment of the actual argument, tested on 
IA-64/Linux and SPARC/Solaris, OK for mainline?


2008-10-31  Eric Botcazou  <ebotcazou@adacore.com>

        * calls.c (store_unaligned_arguments_into_pseudos): Also look into the
        actual alignment of the argument if it lives in memory.


2008-10-31  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/pack11.adb: New test.


-- 
Eric Botcazou

[-- Attachment #2: q.adb --]
[-- Type: text/x-adasrc, Size: 382 bytes --]

-- { dg-do run }
-- { dg-options "-gnatws" }

with System;

procedure Q is

  type R1 is record
    A1, A2, A3 : System.Address;
  end record;

  type R2 is record
    C : Character;
    R : R1;
  end record;
  pragma Pack (R2);

  procedure Dummy (R : R1) is begin null; end;

  procedure Init (X : R2) is
  begin
    Dummy (X.R);
  end;

  My_R2 : R2;

begin
  Init (My_R2);
end;

[-- Attachment #3: p.diff --]
[-- Type: text/x-diff, Size: 855 bytes --]

Index: calls.c
===================================================================
--- calls.c	(revision 141459)
+++ calls.c	(working copy)
@@ -832,13 +832,14 @@ restore_fixed_argument_area (rtx save_ar
 static void
 store_unaligned_arguments_into_pseudos (struct arg_data *args, int num_actuals)
 {
+  const unsigned int ralign = MIN (BIGGEST_ALIGNMENT, BITS_PER_WORD);
   int i, j;
 
   for (i = 0; i < num_actuals; i++)
     if (args[i].reg != 0 && ! args[i].pass_on_stack
 	&& args[i].mode == BLKmode
-	&& (TYPE_ALIGN (TREE_TYPE (args[i].tree_value))
-	    < (unsigned int) MIN (BIGGEST_ALIGNMENT, BITS_PER_WORD)))
+	&& (TYPE_ALIGN (TREE_TYPE (args[i].tree_value)) < ralign
+	    || (MEM_P (args[i].value) && MEM_ALIGN (args[i].value) < ralign)))
       {
 	int bytes = int_size_in_bytes (TREE_TYPE (args[i].tree_value));
 	int endian_correction = 0;

Re: [PATCH] Fix unaligned load on the IA-64

[Richard Guenther]

On Fri, Oct 31, 2008 at 12:25 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> for the testcase:
>
>   type R1 is record
>     A1, A2, A3 : System.Address;
>   end record;
>
>   type R2 is record
>     C : Character;
>     R : R1;
>   end record;
>   pragma Pack (R2);
>
>   procedure Dummy (R : R1) is begin null; end;
>
>   procedure Init (X : R2) is
>   begin
>     Dummy (X.R);
>   end;
>
> R1 is passed in registers on the IA-64 and for Dummy (X.R) the compiler
> (move_block_to_reg) generates an unaligned word move:
>
> (insn 9 8 10 3 q.adb:22 (set (reg:DI 120 out0)
>        (mem/s/j:DI (reg/f:DI 341) [0 S8 A8])) -1 (nil))
>
> (gdb) frame 1
> #1  0x00000000007baa32 in load_register_parameters (args=0x7fffffff9c30,
>    num_actuals=1, call_fusage=0x7fffffff9db8, flags=0, is_sibcall=0,
>    sibcall_failure=0x7fffffff9d8c) at /home/eric/svn/gcc/gcc/calls.c:1697
> 1697                    move_block_to_reg (REGNO (reg), mem, nregs,
> args[i].mode);
> (gdb) p debug_rtx(reg)
> (reg:BLK 120 out0)
> $1 = void
> (gdb) p debug_rtx(mem)
> (mem/s/j:BLK (reg/f:DI 341) [0 S24 A8])
>
>
> The calls.c code contains a machinery that could prevent that from happening:
>
> /* If any elements in ARGS refer to parameters that are to be passed in
>    registers, but not in memory, and whose alignment does not permit a
>    direct copy into registers.  Copy the values into a group of pseudos
>    which we will later copy into the appropriate hard registers.
>
>    Pseudos for each unaligned argument will be stored into the array
>    args[argnum].aligned_regs.  The caller is responsible for deallocating
>    the aligned_regs array if it is nonzero.  */
>
> static void
> store_unaligned_arguments_into_pseudos (struct arg_data *args, int
> num_actuals)
> {
>   int i, j;
>
>   for (i = 0; i < num_actuals; i++)
>     if (args[i].reg != 0 && ! args[i].pass_on_stack
>         && args[i].mode == BLKmode
>         && (TYPE_ALIGN (TREE_TYPE (args[i].tree_value))
>             < (unsigned int) MIN (BIGGEST_ALIGNMENT, BITS_PER_WORD)))
>
> but it doesn't kick in since the type is sufficiently aligned:
>
> (gdb) p debug_tree(args[i]->tree_value)
>    type <record_type 0x2aaaab226a80 q__r1 sizes-gimplified BLK
>        size <integer_cst 0x2aaaab180b40 constant 192>
>        unit size <integer_cst 0x2aaaab2399f0 constant 24>
>        align 64 symtab 0 alias set -1 canonical type 0x2aaaab226a80
>
> although the actual argument isn't:
>
> (gdb) p debug_rtx(args[i]->value)
> (mem/s/j:BLK (reg/f:DI 341) [0 S24 A8])
>
> because of the packedness.
>
>
> Fixed by adding a check on the alignment of the actual argument, tested on
> IA-64/Linux and SPARC/Solaris, OK for mainline?

I wonder if after the args[i].mode == BLKmode check we always have a MEM_P?
Or, if not, if we only ever need the unaligned handling for MEM_Ps?

Otherwise this patch is obviously ok, but maybe we can simplify the logic here.

Thanks,
Richard.

>
> 2008-10-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * calls.c (store_unaligned_arguments_into_pseudos): Also look into the
>        actual alignment of the argument if it lives in memory.
>
>
> 2008-10-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * gnat.dg/pack11.adb: New test.
>
>
> --
> Eric Botcazou
>

Re: [PATCH] Fix unaligned load on the IA-64

[Eric Botcazou]

> I wonder if after the args[i].mode == BLKmode check we always have a MEM_P?
> Or, if not, if we only ever need the unaligned handling for MEM_Ps?

Yes, I pondered that a little too, but the parameter passing code is very 
intricate so I think we should err on the side of caution.  I think testing 
MEM_P before accessing MEM_ALIGN must be kept in any cases, but we could try 
to rely exclusively on MEM_ALIGN and not test TYPE_ALIGN at all.

-- 
Eric Botcazou

Re: [PATCH] Fix unaligned load on the IA-64

[Eric Botcazou]

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

> I wonder if after the args[i].mode == BLKmode check we always have a MEM_P?
> Or, if not, if we only ever need the unaligned handling for MEM_Ps?

The following appears to work, tested on SPARC and IA-64.  OK for mainline?


2008-11-10  Eric Botcazou  <ebotcazou@adacore.com>

        * calls.c (store_unaligned_arguments_into_pseudos): Deal only with
	values living in memory and use more precise alignment information.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 568 bytes --]

Index: calls.c
===================================================================
--- calls.c	(revision 141707)
+++ calls.c	(working copy)
@@ -837,7 +837,8 @@ store_unaligned_arguments_into_pseudos (
   for (i = 0; i < num_actuals; i++)
     if (args[i].reg != 0 && ! args[i].pass_on_stack
 	&& args[i].mode == BLKmode
-	&& (TYPE_ALIGN (TREE_TYPE (args[i].tree_value))
+	&& MEM_P (args[i].value)
+	&& (MEM_ALIGN (args[i].value)
 	    < (unsigned int) MIN (BIGGEST_ALIGNMENT, BITS_PER_WORD)))
       {
 	int bytes = int_size_in_bytes (TREE_TYPE (args[i].tree_value));

Re: [PATCH] Fix unaligned load on the IA-64

[Richard Guenther]

On Mon, Nov 10, 2008 at 6:32 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I wonder if after the args[i].mode == BLKmode check we always have a MEM_P?
>> Or, if not, if we only ever need the unaligned handling for MEM_Ps?
>
> The following appears to work, tested on SPARC and IA-64.  OK for mainline?

Ok.

Thanks,
Richard.

>
> 2008-11-10  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * calls.c (store_unaligned_arguments_into_pseudos): Deal only with
>        values living in memory and use more precise alignment information.
>
>
> --
> Eric Botcazou
>