diff mbox

PATCH: PR target/61296: Excessive alignment in ix86_data_alignment

Message ID 20141217131036.GA18450@gmail.com
State New
Headers show

Commit Message

H.J. Lu Dec. 17, 2014, 1:10 p.m. UTC 
> From: Jakub Jelinek <jakub@redhat.com>
> 
> On Wed, Dec 17, 2014 at 09:33:23AM +0100, Uros Bizjak wrote:
> > Oops, I was under impression that HJ sent the patch on public list.
> 
> The ix86_data_alignment change looks too ugly and too complicated to me.
> 
> I think it would be much cleaner to add after:
> 
>   /* GCC 4.8 and earlier used to incorrectly assume this alignment even
>      for symbols from other compilation units or symbols that don't need
>      to bind locally.  In order to preserve some ABI compatibility with
>      those compilers, ensure we don't decrease alignment from what we
>      used to assume.  */
> 
>   int max_align_compat
>     = optimize_size ? BITS_PER_WORD : MIN (256, MAX_OFILE_ALIGNMENT);
> 
>   /* A data structure, equal or greater than the size of a cache line
>      (64 bytes in the Pentium 4 and other recent Intel processors, including
>      processors based on Intel Core microarchitecture) should be aligned
>      so that its base address is a multiple of a cache line size.  */
> 
>   int max_align
>     = MIN ((unsigned) ix86_tune_cost->prefetch_block * 8, MAX_OFILE_ALIGNMENT);
> 
>   if (max_align < BITS_PER_WORD)
>     max_align = BITS_PER_WORD;
> 
> a new hunk:
> 
>   switch (ix86_align_data_type)
>     {
>     case ix86_align_data_type_abi: opt = false; break;
>     case ix86_align_data_type_compat: max_align = BITS_PER_WORD; break;
>     case ix86_align_data_type_cacheline: break;
>     }
> 
> which would ensure that for the first one we don't do anything beyond ABI
> requirements and for the default one we disregard max_align.
> 
> BTW, supposedly max_align_compat should be just MIN (256, MAX_OFILE_ALIGNMENT);
> unconditionally, now that the user has a way to way to express what he
> wants, we might want to have also -Os built code with newer compilers compat
> ABI backwards compatible with -O2 etc. built objects built by older
> compilers.
> 
> And, at some point in the future, we might consider changing the default
> of this new switch from compat to abi (say in 2-3 years).
> 
>         Jakub

Here is the updated patch.  OK for trunk?

Thanks.

H.J.
---
Add -malign-data={abi|compat,cachineline} to control how GCC aligns
variables.  "compat" uses increased alignment value compatible with
GCC 4.8 and earlier, "abi" uses alignment value as specified by the
psABI, and "cacheline" with increased alignment value to match the
cache line size.  "compat" is the default.

gcc/

	PR target/61296
	* config/i386/i386-opts.h (ix86_align_data): New enum.
	* config/i386/i386.c (ix86_data_alignment): Return the ABI
	alignment value for -malign-data=abi, the cachine line size
	for -malign-data=cachineline and the older GCC compatible
	alignment value for for -malign-data=compat.
	* config/i386/i386.opt (malign-data=): New.
	* doc/invoke.texi: Document -malign-data=.

gcc/testsuite/

	PR target/61296
	* gcc.target/i386/pr61296-2.c: New.
	* gcc.target/i386/pr61296-2.c: Likewise.
	* gcc.target/i386/pr61296-3.c: Likewise.
	* gcc.target/i386/pr61296-4.c: Likewise.
	* gcc.target/i386/pr61296-5.c: Likewise.
	* gcc.target/i386/pr61296-6.c: Likewise.
	* gcc.target/i386/pr61296-7.c: Likewise.
---
 gcc/ChangeLog                             | 13 +++++++++++++
 gcc/config/i386/i386-opts.h               |  6 ++++++
 gcc/config/i386/i386.c                    | 10 ++++++++--
 gcc/config/i386/i386.opt                  | 17 +++++++++++++++++
 gcc/doc/invoke.texi                       | 10 +++++++++-
 gcc/testsuite/ChangeLog                   | 11 +++++++++++
 gcc/testsuite/gcc.target/i386/pr61296-1.c | 27 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr61296-2.c | 27 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr61296-3.c | 27 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr61296-4.c | 27 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr61296-5.c | 27 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr61296-6.c | 27 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr61296-7.c | 27 +++++++++++++++++++++++++++
 13 files changed, 253 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr61296-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr61296-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr61296-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr61296-4.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr61296-5.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr61296-6.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr61296-7.c

Comments

Jakub Jelinek Dec. 17, 2014, 1:14 p.m. UTC | #1 
On Wed, Dec 17, 2014 at 05:10:36AM -0800, H.J. Lu wrote:
> 	PR target/61296
> 	* config/i386/i386-opts.h (ix86_align_data): New enum.
> 	* config/i386/i386.c (ix86_data_alignment): Return the ABI
> 	alignment value for -malign-data=abi, the cachine line size
> 	for -malign-data=cachineline and the older GCC compatible
> 	alignment value for for -malign-data=compat.
> 	* config/i386/i386.opt (malign-data=): New.
> 	* doc/invoke.texi: Document -malign-data=.
> 
> gcc/testsuite/
> 
> 	PR target/61296
> 	* gcc.target/i386/pr61296-2.c: New.
> 	* gcc.target/i386/pr61296-2.c: Likewise.
> 	* gcc.target/i386/pr61296-3.c: Likewise.
> 	* gcc.target/i386/pr61296-4.c: Likewise.
> 	* gcc.target/i386/pr61296-5.c: Likewise.
> 	* gcc.target/i386/pr61296-6.c: Likewise.
> 	* gcc.target/i386/pr61296-7.c: Likewise.

> --- a/gcc/config/i386/i386.opt
> +++ b/gcc/config/i386/i386.opt
> @@ -221,6 +221,23 @@ malign-stringops
>  Target RejectNegative Report InverseMask(NO_ALIGN_STRINGOPS, ALIGN_STRINGOPS) Save
>  Align destination of the string operations
>  
> +malign-data=
> +Target RejectNegative Joined Var(ix86_align_data_type) Enum(ix86_align_data) Init(ix86_align_data_type_compat)
> +Use the given data alignment
> +
> +Enum
> +Name(ix86_align_data) Type(enum ix86_align_data)
> +Known data alignment choices (for use with the -malign-data== option):

Only one = ?

Otherwise LGTM, but I'll defer to Uros for final ack.

	Jakub
Uros Bizjak Dec. 17, 2014, 2:02 p.m. UTC | #2 
On Wed, Dec 17, 2014 at 2:14 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 17, 2014 at 05:10:36AM -0800, H.J. Lu wrote:
>>       PR target/61296
>>       * config/i386/i386-opts.h (ix86_align_data): New enum.
>>       * config/i386/i386.c (ix86_data_alignment): Return the ABI
>>       alignment value for -malign-data=abi, the cachine line size
>>       for -malign-data=cachineline and the older GCC compatible
>>       alignment value for for -malign-data=compat.
>>       * config/i386/i386.opt (malign-data=): New.
>>       * doc/invoke.texi: Document -malign-data=.
>>
>> gcc/testsuite/
>>
>>       PR target/61296
>>       * gcc.target/i386/pr61296-2.c: New.
>>       * gcc.target/i386/pr61296-2.c: Likewise.
>>       * gcc.target/i386/pr61296-3.c: Likewise.
>>       * gcc.target/i386/pr61296-4.c: Likewise.
>>       * gcc.target/i386/pr61296-5.c: Likewise.
>>       * gcc.target/i386/pr61296-6.c: Likewise.
>>       * gcc.target/i386/pr61296-7.c: Likewise.
>
> Otherwise LGTM, but I'll defer to Uros for final ack.

... psABI, and "cacheline" with increased alignment value to match the
cache line size.  "compat" is the default.

s/with/uses/

The patch is OK for mainline with above substitution.

Thanks to everybody involved,
Uros.
Uros Bizjak Dec. 17, 2014, 2:31 p.m. UTC | #3 
On Wed, Dec 17, 2014 at 2:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

> Add -malign-data={abi|compat,cachineline} to control how GCC aligns
> variables.  "compat" uses increased alignment value compatible with
> GCC 4.8 and earlier, "abi" uses alignment value as specified by the
> psABI, and "cacheline" with increased alignment value to match the
> cache line size.  "compat" is the default.
>
> gcc/
>
>         PR target/61296
>         * config/i386/i386-opts.h (ix86_align_data): New enum.
>         * config/i386/i386.c (ix86_data_alignment): Return the ABI
>         alignment value for -malign-data=abi, the cachine line size
>         for -malign-data=cachineline and the older GCC compatible
>         alignment value for for -malign-data=compat.
>         * config/i386/i386.opt (malign-data=): New.
>         * doc/invoke.texi: Document -malign-data=.

Please also mention new user-visible options in GCC changes document.

(Oh, just spotted a trivial typo in the ChangeLog entry: cachineline
-> cacheline).

Uros.
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 7b3c4aa..663669b 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,16 @@ 
+2014-12-17  H.J. Lu  <hongjiu.lu@intel.com>
+	    Jakub Jelinek  <jakub@redhat.com>
+	    Uros Bizjak  <ubizjak@gmail.com>
+
+	PR target/61296
+	* config/i386/i386-opts.h (ix86_align_data): New enum.
+	* config/i386/i386.c (ix86_data_alignment): Return the ABI
+	alignment value for -malign-data=abi, the cachine line size
+	for -malign-data=cachineline and the older GCC compatible
+	alignment value for for -malign-data=compat.
+	* config/i386/i386.opt (malign-data=): New.
+	* doc/invoke.texi: Document -malign-data=.
+
 2014-12-17  Marek Polacek  <polacek@redhat.com>
 
 	PR middle-end/63568
diff --git a/gcc/config/i386/i386-opts.h b/gcc/config/i386/i386-opts.h
index 47a34db..455df43 100644
--- a/gcc/config/i386/i386-opts.h
+++ b/gcc/config/i386/i386-opts.h
@@ -77,6 +77,12 @@  enum pmode {
   PMODE_DI 	/* Pmode == DImode. */
 };
 
+enum ix86_align_data {
+  ix86_align_data_type_compat,
+  ix86_align_data_type_abi,
+  ix86_align_data_type_cacheline
+};
+
 enum asm_dialect {
   ASM_ATT,
   ASM_INTEL
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 72c1219..17ef751 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -27191,8 +27191,7 @@  ix86_data_alignment (tree type, int align, bool opt)
      those compilers, ensure we don't decrease alignment from what we
      used to assume.  */
 
-  int max_align_compat
-    = optimize_size ? BITS_PER_WORD : MIN (256, MAX_OFILE_ALIGNMENT);
+  int max_align_compat = MIN (256, MAX_OFILE_ALIGNMENT);
 
   /* A data structure, equal or greater than the size of a cache line
      (64 bytes in the Pentium 4 and other recent Intel processors, including
@@ -27205,6 +27204,13 @@  ix86_data_alignment (tree type, int align, bool opt)
   if (max_align < BITS_PER_WORD)
     max_align = BITS_PER_WORD;
 
+  switch (ix86_align_data_type)
+    {
+    case ix86_align_data_type_abi: opt = false; break;
+    case ix86_align_data_type_compat: max_align = BITS_PER_WORD; break;
+    case ix86_align_data_type_cacheline: break;
+    }
+
   if (opt
       && AGGREGATE_TYPE_P (type)
       && TYPE_SIZE (type)
diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
index b1c6319..d0badda 100644
--- a/gcc/config/i386/i386.opt
+++ b/gcc/config/i386/i386.opt
@@ -221,6 +221,23 @@  malign-stringops
 Target RejectNegative Report InverseMask(NO_ALIGN_STRINGOPS, ALIGN_STRINGOPS) Save
 Align destination of the string operations
 
+malign-data=
+Target RejectNegative Joined Var(ix86_align_data_type) Enum(ix86_align_data) Init(ix86_align_data_type_compat)
+Use the given data alignment
+
+Enum
+Name(ix86_align_data) Type(enum ix86_align_data)
+Known data alignment choices (for use with the -malign-data== option):
+
+EnumValue
+Enum(ix86_align_data) String(compat) Value(ix86_align_data_type_compat)
+
+EnumValue
+Enum(ix86_align_data) String(abi) Value(ix86_align_data_type_abi)
+
+EnumValue
+Enum(ix86_align_data) String(cacheline) Value(ix86_align_data_type_cacheline)
+
 march=
 Target RejectNegative Joined Var(ix86_arch_string)
 Generate code for given CPU
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 19422d7..6e88dce 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -701,7 +701,7 @@  Objective-C and Objective-C++ Dialects}.
 -m32 -m64 -mx32 -m16 -mlarge-data-threshold=@var{num} @gol
 -msse2avx -mfentry -mrecord-mcount -mnop-mcount -m8bit-idiv @gol
 -mavx256-split-unaligned-load -mavx256-split-unaligned-store @gol
--mstack-protector-guard=@var{guard}}
+-malign-data=@var{type} -mstack-protector-guard=@var{guard}}
 
 @emph{i386 and x86-64 Windows Options}
 @gccoptlist{-mconsole -mcygwin -mno-cygwin -mdll @gol
@@ -15682,6 +15682,14 @@  as well as modifying the function calling convention for functions taking
 @code{long double}.  Hence they are not binary-compatible
 with code compiled without that switch.
 
+@item -malign-data=@var{type}
+@opindex malign-data
+Control how GCC aligns variables.  Supported values for @var{type} are
+@samp{compat} with increased alignment value compatible with GCC 4.8
+and earlier, @samp{abi} with alignment value as specified by the
+psABI, and @samp{cacheline} with increased alignment value to match
+the cache line size.  @samp{compat} is the default.
+
 @item -mlarge-data-threshold=@var{threshold}
 @opindex mlarge-data-threshold
 When @option{-mcmodel=medium} is specified, data objects larger than
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 3b04479..24c5143 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,14 @@ 
+2014-12-17  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR target/61296
+	* gcc.target/i386/pr61296-2.c: New.
+	* gcc.target/i386/pr61296-2.c: Likewise.
+	* gcc.target/i386/pr61296-3.c: Likewise.
+	* gcc.target/i386/pr61296-4.c: Likewise.
+	* gcc.target/i386/pr61296-5.c: Likewise.
+	* gcc.target/i386/pr61296-6.c: Likewise.
+	* gcc.target/i386/pr61296-7.c: Likewise.
+
 2014-12-17  Tejas Belagod  <tejas.belagod@arm.com>
 
 	PR testsuite/64328
diff --git a/gcc/testsuite/gcc.target/i386/pr61296-1.c b/gcc/testsuite/gcc.target/i386/pr61296-1.c
new file mode 100644
index 0000000..751dee0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr61296-1.c
@@ -0,0 +1,27 @@ 
+/* PR target/61296 */
+/* { dg-do compile { target { *-*-linux* } } } */
+/* { dg-options "-O2" } */
+
+struct foo
+{
+  char i1[8];
+  char i2[8];
+  char i3[8];
+  char i4[8];
+  char i5[8];
+  char i6[8];
+  char i7[8];
+  char i8[8];
+  char i9[8];
+  char i10[8];
+  char i11[8];
+  char i12[8];
+  char i13[8];
+  char i14[8];
+  char i15[8];
+  char i16[8];
+};
+
+struct foo x = { 1 };
+
+/* { dg-final { scan-assembler ".align\[ \t]*32\[^:]*\[\n\r]x:" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr61296-2.c b/gcc/testsuite/gcc.target/i386/pr61296-2.c
new file mode 100644
index 0000000..5999555
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr61296-2.c
@@ -0,0 +1,27 @@ 
+/* PR target/61296 */
+/* { dg-do compile { target { *-*-linux* } } } */
+/* { dg-options "-O2 -malign-data=cacheline" } */
+
+struct foo
+{
+  char i1[8];
+  char i2[8];
+  char i3[8];
+  char i4[8];
+  char i5[8];
+  char i6[8];
+  char i7[8];
+  char i8[8];
+  char i9[8];
+  char i10[8];
+  char i11[8];
+  char i12[8];
+  char i13[8];
+  char i14[8];
+  char i15[8];
+  char i16[8];
+};
+
+struct foo x = { 1 };
+
+/* { dg-final { scan-assembler ".align\[ \t]*64\[^:]*\[\n\r]x:" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr61296-3.c b/gcc/testsuite/gcc.target/i386/pr61296-3.c
new file mode 100644
index 0000000..d0152f7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr61296-3.c
@@ -0,0 +1,27 @@ 
+/* PR target/61296 */
+/* { dg-do compile { target { *-*-linux* } } } */
+/* { dg-options "-O2 -malign-data=abi" } */
+
+struct foo
+{
+  char i1[8];
+  char i2[8];
+  char i3[8];
+  char i4[8];
+  char i5[8];
+  char i6[8];
+  char i7[8];
+  char i8[8];
+  char i9[8];
+  char i10[8];
+  char i11[8];
+  char i12[8];
+  char i13[8];
+  char i14[8];
+  char i15[8];
+  char i16[8];
+};
+
+struct foo x = { 1 };
+
+/* { dg-final { scan-assembler-not ".align\[ \t]*\[0-9\]+\[^:]*\[\n\r]x:" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr61296-4.c b/gcc/testsuite/gcc.target/i386/pr61296-4.c
new file mode 100644
index 0000000..95e1ac6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr61296-4.c
@@ -0,0 +1,27 @@ 
+/* PR target/61296 */
+/* { dg-do compile { target { *-*-linux* } } } */
+/* { dg-options "-O2 -malign-data=cacheline -malign-data=abi" } */
+
+struct foo
+{
+  char i1[8];
+  char i2[8];
+  char i3[8];
+  char i4[8];
+  char i5[8];
+  char i6[8];
+  char i7[8];
+  char i8[8];
+  char i9[8];
+  char i10[8];
+  char i11[8];
+  char i12[8];
+  char i13[8];
+  char i14[8];
+  char i15[8];
+  char i16[8];
+};
+
+struct foo x = { 1 };
+
+/* { dg-final { scan-assembler-not ".align\[ \t]*\[0-9\]+\[^:]*\[\n\r]x:" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr61296-5.c b/gcc/testsuite/gcc.target/i386/pr61296-5.c
new file mode 100644
index 0000000..5caa77c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr61296-5.c
@@ -0,0 +1,27 @@ 
+/* PR target/61296 */
+/* { dg-do compile { target { *-*-linux* } } } */
+/* { dg-options "-O2 -malign-data=abi -malign-data=cacheline" } */
+
+struct foo
+{
+  char i1[8];
+  char i2[8];
+  char i3[8];
+  char i4[8];
+  char i5[8];
+  char i6[8];
+  char i7[8];
+  char i8[8];
+  char i9[8];
+  char i10[8];
+  char i11[8];
+  char i12[8];
+  char i13[8];
+  char i14[8];
+  char i15[8];
+  char i16[8];
+};
+
+struct foo x = { 1 };
+
+/* { dg-final { scan-assembler ".align\[ \t]*64\[^:]*\[\n\r]x:" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr61296-6.c b/gcc/testsuite/gcc.target/i386/pr61296-6.c
new file mode 100644
index 0000000..8e0d535
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr61296-6.c
@@ -0,0 +1,27 @@ 
+/* PR target/61296 */
+/* { dg-do compile { target { *-*-linux* } } } */
+/* { dg-options "-O2 -malign-data=cacheline -malign-data=compat" } */
+
+struct foo
+{
+  char i1[8];
+  char i2[8];
+  char i3[8];
+  char i4[8];
+  char i5[8];
+  char i6[8];
+  char i7[8];
+  char i8[8];
+  char i9[8];
+  char i10[8];
+  char i11[8];
+  char i12[8];
+  char i13[8];
+  char i14[8];
+  char i15[8];
+  char i16[8];
+};
+
+struct foo x = { 1 };
+
+/* { dg-final { scan-assembler ".align\[ \t]*32\[^:]*\[\n\r]x:" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr61296-7.c b/gcc/testsuite/gcc.target/i386/pr61296-7.c
new file mode 100644
index 0000000..6a67c90
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr61296-7.c
@@ -0,0 +1,27 @@ 
+/* PR target/61296 */
+/* { dg-do compile { target { *-*-linux* } } } */
+/* { dg-options "-O2 -malign-data=compat -malign-data=abi" } */
+
+struct foo
+{
+  char i1[8];
+  char i2[8];
+  char i3[8];
+  char i4[8];
+  char i5[8];
+  char i6[8];
+  char i7[8];
+  char i8[8];
+  char i9[8];
+  char i10[8];
+  char i11[8];
+  char i12[8];
+  char i13[8];
+  char i14[8];
+  char i15[8];
+  char i16[8];
+};
+
+struct foo x = { 1 };
+
+/* { dg-final { scan-assembler-not ".align\[ \t]*\[0-9\]+\[^:]*\[\n\r]x:" } } */