[Bug target/18916] New: vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[Bug target/18916] New: vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[fjahanian at apple dot com]

Following test case, compiled with -mcpu=G5, aborts.
It aborts because in passing the 32-byte argument (g1sScld1) to testvaScld1 routine
gcc allocates a temporary on the stack for the purpose of storing
g1sScld1 and then loading it into GPRs. Recently, rs6000.c
was modified in routine expand_block_move to do lvx/stvx when alignment
of src and destination are 128 bits. But in the case of temporaries allocated
on the stack, target alignment is not correct. It is true that we set the MEM_ALIGN
of target temporary to 128 bit, but it comes from the alignment of the source
which is a user variable and has the 128 bit alignment.

So, in the given test case, routine expand_block_move generates
stvx to temporary stack location which is misaligned and bad things happen.

extern void abort (void);

typedef __builtin_va_list __gnuc_va_list;
typedef __gnuc_va_list va_list;
    
typedef struct { _Complex long double a; } Scld1;

void testvaScld1 (int n, ...)
{   
  va_list ap;
    
   __builtin_va_start(ap,n);

   Scld1 t = __builtin_va_arg(ap,Scld1);

   if (t.a != (_Complex long double)1)
     abort();

   __builtin_va_end(ap);
}

int main ()
{
  Scld1 g1sScld1;
  g1sScld1.a = (_Complex long double)1;
  testvaScld1 (1, g1sScld1);
  return 0;
}

-- 
           Summary: vector code is generated to copy data to mis-aligned
                    memory (-mcpu=G5)
           Product: gcc
           Version: 4.0.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: fjahanian at apple dot com
                CC: gcc-bugs at gcc dot gnu dot org
 GCC build triplet: apple-ppc-darwin
  GCC host triplet: apple-ppc-darwin
GCC target triplet: apple-ppc-darwin


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[fjahanian at apple dot com]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dje at watson dot ibm dot
                   |                            |com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-12-10 01:27 -------
Might be related to PR 17990.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[fjahanian at apple dot com]

------- Additional Comments From fjahanian at apple dot com  2004-12-10 01:42 -------
AFAICT, I don't see how gcc middle-end can force correct parameter alignment when alignment is more
strict than PARAM_BOUNDARY. There is no code to do so (I am looking at store_one_arg which is
the one responsible for determining the alignment). It does set the MEM_ALIGN field to 128 in this 
case, but there is no extra padding to move the target address to the next 128 bit boundary.

 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[dje at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |geoffk at apple dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-12-10 03:35 -------
This worked in 4.0 because expand_block_move did not use altivec before.

Confirmed.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|                            |1
           Keywords|                            |wrong-code
   Last reconfirmed|0000-00-00 00:00:00         |2004-12-10 03:35:51
               date|                            |
            Summary|vector code is generated to |[4.0 Regression] vector code
                   |copy data to mis-aligned    |is generated to copy data to
                   |memory (-mcpu=G5)           |mis-aligned memory (-
                   |                            |mcpu=G5)
   Target Milestone|---                         |4.0.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] vector code is generated to copy data to mis-aligned memory (-mcpu=G5)

[pinskia at gcc dot gnu dot org]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pinskia at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] vector code is generated to copy data to mis-aligned memory (-maltivec)

[pinskia at gcc dot gnu dot org]

------- Additional Comments From pinskia at gcc dot gnu dot org  2004-12-14 01:48 -------
It also fails with -mpu=G4.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[4.0 Regression] vector code|[4.0 Regression] vector code
                   |is generated to copy data to|is generated to copy data to
                   |mis-aligned memory (-       |mis-aligned memory (-
                   |mcpu=G5)                    |maltivec)


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[fjahanian at apple dot com]

------- Additional Comments From fjahanian at apple dot com  2004-12-18 00:43 -------
Followin patch fixes the alignment problem. But it cannot be applied because it breaks ABI
compatibilty. 

A possible solution is to relax alignment of the type in question (with alignment of 128) to that of the
PARM_BOUNDARY (32). This will not (should not ?) break the ABI compatibility (because it is currently 
on PARM_BOUNDARY). But it will prevent vector code to be generated (which is cause of the abort).
Comments are most welcome.


Index: rs6000.c
===============================================================
====
RCS file: /cvs/gcc/gcc/gcc/config/rs6000/rs6000.c,v
retrieving revision 1.332.2.46.2.84
diff -c -p -r1.332.2.46.2.84 rs6000.c
*** rs6000.c    16 Dec 2004 03:23:30 -0000      1.332.2.46.2.84
--- rs6000.c    18 Dec 2004 00:20:54 -0000
*************** function_arg_boundary (enum machine_mode
*** 5190,5195 ****
--- 5190,5197 ----
           || (type && TREE_CODE (type) == VECTOR_TYPE
               && int_size_in_bytes (type) >= 16))
      return 128;
+   else if (DEFAULT_ABI == ABI_DARWIN && mode == BLKmode)
+     return MAX (TYPE_ALIGN (type), PARM_BOUNDARY);
    else
      return PARM_BOUNDARY;
  }

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[fjahanian at apple dot com]

------- Additional Comments From fjahanian at apple dot com  2004-12-18 01:46 -------
And this is the patch that I had in mind. Can this break ABI compatibily? My limited testing shows
that it does not.

Index: rs6000.c
===============================================================
====
RCS file: /cvs/gcc/gcc/gcc/config/rs6000/rs6000.c,v
retrieving revision 1.332.2.46.2.84
diff -c -p -r1.332.2.46.2.84 rs6000.c
*** rs6000.c    16 Dec 2004 03:23:30 -0000      1.332.2.46.2.84
--- rs6000.c    18 Dec 2004 01:44:28 -0000
*************** function_arg_boundary (enum machine_mode
*** 5190,5195 ****
--- 5190,5201 ----
           || (type && TREE_CODE (type) == VECTOR_TYPE
               && int_size_in_bytes (type) >= 16))
      return 128;
+   else if (DEFAULT_ABI == ABI_DARWIN && mode == BLKmode
+          && TYPE_ALIGN (type) >= 128)
+     {
+       TYPE_ALIGN (type) = PARM_BOUNDARY;
+       return PARM_BOUNDARY;
+     }
    else
      return PARM_BOUNDARY;
  }

(In reply to comment #5)
> Followin patch fixes the alignment problem. But it cannot be applied because it breaks ABI
> compatibilty. 
> 
> A possible solution is to relax alignment of the type in question (with alignment of 128) to that of the
> PARM_BOUNDARY (32). This will not (should not ?) break the ABI compatibility (because it is currently 
> on PARM_BOUNDARY). But it will prevent vector code to be generated (which is cause of the abort).
> Comments are most welcome.
> 
> 
> Index: rs6000.c
> 
===============================================================
> ====
> RCS file: /cvs/gcc/gcc/gcc/config/rs6000/rs6000.c,v
> retrieving revision 1.332.2.46.2.84
> diff -c -p -r1.332.2.46.2.84 rs6000.c
> *** rs6000.c    16 Dec 2004 03:23:30 -0000      1.332.2.46.2.84
> --- rs6000.c    18 Dec 2004 00:20:54 -0000
> *************** function_arg_boundary (enum machine_mode
> *** 5190,5195 ****
> --- 5190,5197 ----
>            || (type && TREE_CODE (type) == VECTOR_TYPE
>                && int_size_in_bytes (type) >= 16))
>       return 128;
> +   else if (DEFAULT_ABI == ABI_DARWIN && mode == BLKmode)
> +     return MAX (TYPE_ALIGN (type), PARM_BOUNDARY);
>     else
>       return PARM_BOUNDARY;
>   }



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[fjahanian at apple dot com]

------- Additional Comments From fjahanian at apple dot com  2004-12-21 01:25 -------
My last patch also had problems, in that it changed alignment of local vector variables on  stack.
This alignment cannot be changed because AltiVec intrincics expect 128bit alignment. So,
I conclude that only tempoaries with expected 128bit or more alignments are not aligned
properly. The safest fix would be to simply change the alignment at the rtl level when
temporaries of 128bit alignment need be generated. This requires change to the middle end.
Following patch shows the concept and is not an FSF ready patch (which requires target-hook or
some such). This patch essentially says that if 128 alignment of local temporaries on stack can not
be guaranteed (or changed), then set the alignment value in the rtl to what can be guaranteed. 
With this patch, emit_block_move will not generate lvx/stvx for these cases.

Index: expr.c
===============================================================
====
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.761
diff -c -p -r1.761 expr.c
*** expr.c      18 Dec 2004 14:38:31 -0000      1.761
--- expr.c      21 Dec 2004 01:23:27 -0000
*************** emit_push_insn (rtx x, enum machine_mode
*** 3457,3463 ****
             to record the alignment of the stack slot.  */
          /* ALIGN may well be better aligned than TYPE, e.g. due to
             PARM_BOUNDARY.  Assume the caller isn't lying.  */
!         set_mem_align (target, align);
  
          emit_block_move (target, xinner, size, BLOCK_OP_CALL_PARM);
        }
--- 3457,3469 ----
             to record the alignment of the stack slot.  */
          /* ALIGN may well be better aligned than TYPE, e.g. due to
             PARM_BOUNDARY.  Assume the caller isn't lying.  */
!           /* powerpc-darwin currently does not enforce 128 bit alignment of 
!              temporaries on the stack. To do so, requires changes which will break
!              ABI compatibility. On the other hand, Leaving this unchanged generates 
!              incorrect code in cases where block move is implemented using
!              AltiVec instructions whose src and dest must be 128 bit aligned
!              (expand_block_move implementation in rs6000.c). */ 
!           set_mem_align (target, align >= 128 ? PARM_BOUNDARY : align);
  
          emit_block_move (target, xinner, size, BLOCK_OP_CALL_PARM);
        }
*************** store_expr (tree exp, rtx target, int ca
*** 4206,4214 ****
        emit_group_load (target, temp, TREE_TYPE (exp),
                         int_size_in_bytes (TREE_TYPE (exp)));
        else if (GET_MODE (temp) == BLKmode)
!       emit_block_move (target, temp, expr_size (exp),
!                        (call_param_p
!                         ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
        else
        {
          temp = force_operand (temp, target);
--- 4212,4224 ----
        emit_group_load (target, temp, TREE_TYPE (exp),
                         int_size_in_bytes (TREE_TYPE (exp)));
        else if (GET_MODE (temp) == BLKmode)
!         {
!           /* See previous comment. */
!           set_mem_align (temp, MEM_ALIGN (temp) >= 128 ? PARM_BOUNDARY : MEM_ALIGN (temp));
!           emit_block_move (target, temp, expr_size (exp),
!                            (call_param_p
!                             ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
!         }
        else
        {
          temp = force_operand (temp, target);




(In reply to comment #6)
> And this is the patch that I had in mind. Can this break ABI compatibily? My limited testing shows
> that it does not.
> 
> Index: rs6000.c
> 
===============================================================
> ====
> RCS file: /cvs/gcc/gcc/gcc/config/rs6000/rs6000.c,v
> retrieving revision 1.332.2.46.2.84
> diff -c -p -r1.332.2.46.2.84 rs6000.c
> *** rs6000.c    16 Dec 2004 03:23:30 -0000      1.332.2.46.2.84
> --- rs6000.c    18 Dec 2004 01:44:28 -0000
> *************** function_arg_boundary (enum machine_mode
> *** 5190,5195 ****
> --- 5190,5201 ----
>            || (type && TREE_CODE (type) == VECTOR_TYPE
>                && int_size_in_bytes (type) >= 16))
>       return 128;
> +   else if (DEFAULT_ABI == ABI_DARWIN && mode == BLKmode
> +          && TYPE_ALIGN (type) >= 128)
> +     {
> +       TYPE_ALIGN (type) = PARM_BOUNDARY;
> +       return PARM_BOUNDARY;
> +     }
>     else
>       return PARM_BOUNDARY;
>   }
> 

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dalej at apple dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[geoffk at gcc dot gnu dot org]

------- Additional Comments From geoffk at gcc dot gnu dot org  2004-12-28 23:05 -------
Why can't we make sure that temporaries which should be aligned to 128 bits are actually aligned to 
128 bits?  Surely failing to do so will cause other problems.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[fjahanian at apple dot com]

------- Additional Comments From fjahanian at apple dot com  2004-12-29 17:34 -------

(In reply to comment #8)
> Why can't we make sure that temporaries which should be aligned to 128 bits are actually aligned to 
> 128 bits?  Surely failing to do so will cause other problems.

Yes, this is the best way of fixing this problem, hoping not to break ABI conformacne in some obscure
way along the way. My last posted patch, took the approach of setting the alignment of the
stack temporaries to what they really were. This worked, but it also turned off the Vector move insns
for such temporaries. I will look at forcing the 128 bit alignment next year.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[amodra at bigpond dot net dot au]

------- Additional Comments From amodra at bigpond dot net dot au  2004-12-31 23:15 -------
This is a problem with the gcc's function call machinery.  The underlying
problem is that mem alignment of function args is taken from the type of the
arg, not the alignment given by function_arg_boundary.  At least, that's what
I'm seeing on investigating why g++.dg/ext/altivec-3.C is failing on
powerpc64-linux, and this looks like the same bug.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |amodra at bigpond dot net
                   |dot org                     |dot au
             Status|NEW                         |ASSIGNED
 GCC target triplet|apple-ppc-darwin            |apple-ppc-darwin, powerpc*-
                   |                            |*-linux
   Last reconfirmed|2004-12-10 03:35:51         |2004-12-31 23:15:05
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[amodra at bigpond dot net dot au]

------- Additional Comments From amodra at bigpond dot net dot au  2005-01-01 07:17 -------
http://gcc.gnu.org/ml/gcc-patches/2005-01/msg00014.html

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[amodra at bigpond dot net dot au]

------- Additional Comments From amodra at bigpond dot net dot au  2005-01-07 23:36 -------
Revised patch
http://gcc.gnu.org/ml/gcc-patches/2005-01/msg00422.html

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2005-01-11 09:51 -------
Subject: Bug 18916

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	amodra@gcc.gnu.org	2005-01-11 09:51:24

Modified files:
	gcc            : ChangeLog builtins.c calls.c expr.h function.c 

Log message:
	PR target/18916
	* builtins.c (std_gimplify_va_arg_expr): Adjust alignment of *ap.
	* expr.h (struct locate_and_pad_arg_data): Add "boundary".
	* function.c (locate_and_pad_parm): Set new field.
	(assign_parm_find_stack_rtl): Use it instead of FUNCTION_ARG_BOUNDARY.
	Tweak where_pad test to include "none".  Always set mem align for
	stack_parm.
	(assign_parm_adjust_stack_rtl): Discard stack_parm if alignment
	not sufficient for type.
	(assign_parm_setup_block): If stack_parm is zero on entry, always
	make a new stack local.  Block move old stack parm if necessary
	to new aligned stack local.
	(assign_parm_setup_stack): Use a block move to handle
	potentially misaligned entry_parm.
	(assign_parms_unsplit_complex): Specify required alignment when
	creating stack local.
	* calls.c (compute_argument_addresses): Override alignment of stack
	arg calculated from its type with the alignment given by
	FUNCTION_ARG_BOUNDARY.
	(store_one_arg): Likewise.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.7090&r2=2.7091
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/builtins.c.diff?cvsroot=gcc&r1=1.411&r2=1.412
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/calls.c.diff?cvsroot=gcc&r1=1.375&r2=1.376
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/expr.h.diff?cvsroot=gcc&r1=1.184&r2=1.185
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/function.c.diff?cvsroot=gcc&r1=1.599&r2=1.600



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916

[Bug target/18916] [4.0 Regression] mis-aligned vector code with copy memory (-maltivec)

[amodra at bigpond dot net dot au]

------- Additional Comments From amodra at bigpond dot net dot au  2005-01-11 10:03 -------
Should now be fixed

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18916