Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)

[Alan Modra <amodra@gmail.com>]

On Thu, Jun 13, 2013 at 05:10:51PM +0930, Alan Modra wrote:
> On Wed, Jun 12, 2013 at 12:52:03PM -0500, Edmar Wienskoski wrote:
> > The e500v2 (SPE) hardware is such that if the address of vector (double world
> > load / stores) are not double world aligned the instruction will trap.
> > 
> > So this alignment is not optional.
> 
> Vector type alignment is also specified by the ppc64 abi.  I think we
> want the following.  Note that DATA_ALIGNMENT has been broken for
> vectors right from the initial vector support (and the error was
> copied for e500 double).  For example
> 
> typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
> vec_align x = { 0, 0, 0, 0 };
> 
> currently loses the extra alignment.  Fixed by never decreasing
> alignment in DATA_ABI_ALIGNMENT.  Testing in progress.  OK to
> apply assuming bootstrap is good?  (I think I need a change in
> offsettable_ok_by_alignment too.  I'll do that in a separate patch.)

Revised patch with offsettable_ok_by_alignment change, avoiding dumb
idea of using statement expressions.  This one actually bootstraps and
passes regression testing.

	* config/rs6000/rs6000.h (enum data_align): New.
	(LOCAL_ALIGNMENT, DATA_ALIGNMENT): Use rs6000_data_alignment.
	(DATA_ABI_ALIGNMENT): Define.
	(CONSTANT_ALIGNMENT): Correct comment.
	* config/rs6000/rs6000-protos.h (rs6000_data_alignment): Declare.
	* config/rs6000/rs6000.c (rs6000_data_alignment): New function.
	(offsettable_ok_by_alignment): Align by DATA_ABI_ALIGNMENT.
	Pass "type" not "decl" to DATA_ALIGNMENT.

Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 200055)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -813,12 +813,6 @@ extern unsigned rs6000_pointer_size;
 /* No data type wants to be aligned rounder than this.  */
 #define BIGGEST_ALIGNMENT 128
 
-/* A C expression to compute the alignment for a variables in the
-   local store.  TYPE is the data type, and ALIGN is the alignment
-   that the object would ordinarily have.  */
-#define LOCAL_ALIGNMENT(TYPE, ALIGN)				\
-  DATA_ALIGNMENT (TYPE, ALIGN)
-
 /* Alignment of field after `int : 0' in a structure.  */
 #define EMPTY_FIELD_BOUNDARY 32
 
@@ -828,8 +822,15 @@ extern unsigned rs6000_pointer_size;
 /* A bit-field declared as `int' forces `int' alignment for the struct.  */
 #define PCC_BITFIELD_TYPE_MATTERS 1
 
-/* Make strings word-aligned so strcpy from constants will be faster.
-   Make vector constants quadword aligned.  */
+enum data_align { align_abi, align_opt, align_both };
+
+/* A C expression to compute the alignment for a variables in the
+   local store.  TYPE is the data type, and ALIGN is the alignment
+   that the object would ordinarily have.  */
+#define LOCAL_ALIGNMENT(TYPE, ALIGN)				\
+  rs6000_data_alignment (TYPE, ALIGN, align_both)
+
+/* Make strings word-aligned so strcpy from constants will be faster.  */
 #define CONSTANT_ALIGNMENT(EXP, ALIGN)                           \
   (TREE_CODE (EXP) == STRING_CST	                         \
    && (STRICT_ALIGNMENT || !optimize_size)                       \
@@ -837,21 +838,14 @@ extern unsigned rs6000_pointer_size;
    ? BITS_PER_WORD                                               \
    : (ALIGN))
 
-/* Make arrays of chars word-aligned for the same reasons.
-   Align vectors to 128 bits.  Align SPE vectors and E500 v2 doubles to
+/* Make arrays of chars word-aligned for the same reasons.  */
+#define DATA_ALIGNMENT(TYPE, ALIGN) \
+  rs6000_data_alignment (TYPE, ALIGN, align_opt)
+
+/* Align vectors to 128 bits.  Align SPE vectors and E500 v2 doubles to
    64 bits.  */
-#define DATA_ALIGNMENT(TYPE, ALIGN)					\
-  (TREE_CODE (TYPE) == VECTOR_TYPE					\
-   ? (((TARGET_SPE && SPE_VECTOR_MODE (TYPE_MODE (TYPE)))		\
-       || (TARGET_PAIRED_FLOAT && PAIRED_VECTOR_MODE (TYPE_MODE (TYPE)))) \
-      ? 64 : 128)							\
-   : ((TARGET_E500_DOUBLE						\
-       && TREE_CODE (TYPE) == REAL_TYPE					\
-       && TYPE_MODE (TYPE) == DFmode)					\
-      ? 64								\
-      : (TREE_CODE (TYPE) == ARRAY_TYPE					\
-	 && TYPE_MODE (TREE_TYPE (TYPE)) == QImode			\
-	 && (ALIGN) < BITS_PER_WORD) ? BITS_PER_WORD : (ALIGN)))
+#define DATA_ABI_ALIGNMENT(TYPE, ALIGN) \
+  rs6000_data_alignment (TYPE, ALIGN, align_abi)
 
 /* Nonzero if move instructions will actually fail to work
    when given unaligned data.  */
Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 200055)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -141,6 +141,7 @@ extern int rs6000_loop_align (rtx);
 #endif /* RTX_CODE */
 
 #ifdef TREE_CODE
+extern unsigned int rs6000_data_alignment (tree, unsigned int, enum data_align);
 extern unsigned int rs6000_special_round_type_align (tree, unsigned int,
 						     unsigned int);
 extern unsigned int darwin_rs6000_special_round_type_align (tree, unsigned int,
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 200055)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -5384,6 +5390,44 @@ invalid_e500_subreg (rtx op, enum machine_mode mod
   return false;
 }
 
+/* Return alignment of TYPE.  Existing alignment is ALIGN.  HOW
+   selects whether the alignment is abi mandated, optional, or
+   both abi and optional alignment.  */
+   
+unsigned int
+rs6000_data_alignment (tree type, unsigned int align, enum data_align how)
+{
+  if (how != align_opt)
+    {
+      if (TREE_CODE (type) == VECTOR_TYPE)
+	{
+	  if ((TARGET_SPE && SPE_VECTOR_MODE (TYPE_MODE (type)))
+	      || (TARGET_PAIRED_FLOAT && PAIRED_VECTOR_MODE (TYPE_MODE (type))))
+	    {
+	      if (align < 64)
+		align = 64;
+	    }
+	  else if (align < 128)
+	    align = 128;
+	}
+      else if (TARGET_E500_DOUBLE
+	       && TREE_CODE (type) == REAL_TYPE
+	       && TYPE_MODE (type) == DFmode
+	       && align < 64)
+	align = 64;
+    }
+
+  if (how != align_abi)
+    {
+      if (TREE_CODE (type) == ARRAY_TYPE
+	  && TYPE_MODE (TREE_TYPE (type)) == QImode
+	  && align < BITS_PER_WORD)
+	align = BITS_PER_WORD;
+    }
+
+  return align;
+}
+
 /* AIX increases natural record alignment to doubleword if the first
    field is an FP double while the FP fields remain word aligned.  */
 
@@ -5774,10 +5818,11 @@ offsettable_ok_by_alignment (rtx op, HOST_WIDE_INT
       type = TREE_TYPE (decl);
 
       dalign = TYPE_ALIGN (type);
+      dalign = DATA_ABI_ALIGNMENT (type, dalign);
       if (CONSTANT_CLASS_P (decl))
 	dalign = CONSTANT_ALIGNMENT (decl, dalign);
       else
-	dalign = DATA_ALIGNMENT (decl, dalign);
+	dalign = DATA_ALIGNMENT (type, dalign);
 
       if (dsize == 0)
 	{

-- 
Alan Modra
Australia Development Lab, IBM