Re: FW: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
The change also affects vectorizer in avx case which could be seen for gcc.dg/tree-ssa/loop-19.c test. After the change report says loop-19_bad.c:16: note: === vect_analyze_data_refs_alignment === loop-19_bad.c:16: note: vect_compute_data_ref_alignment: loop-19_bad.c:16: note: can't force alignment of ref: a[j_9] loop-19_bad.c:16: note: vect_compute_data_ref_alignment: loop-19_bad.c:16: note: can't force alignment of ref: c[j_9] AFAICS first condition in ix86_data_alignment was true before the change so 256 was a return value. Do we need to tweak this test also? Thanks, Igor Hi! This PR is about DATA_ALIGNMENT macro increasing alignment of some decls for optimization purposes beyond ABI mandated levels. It is fine to emit the vars aligned as much as we want for optimization purposes, but if we can't be sure that references to that decl bind to the definition we increased the alignment on (e.g. common variables, or -fpic code without hidden visibility, weak vars etc.), we can't assume that alignment. As DECL_ALIGN is used for both the alignment emitted for the definitions and alignment assumed on code referring to it, this patch increases DECL_ALIGN only on decls where decl_binds_to_current_def_p is true, and otherwise the optimization part on top of that emits only when aligning definition. On x86_64, DATA_ALIGNMENT macro was partly an optimization, partly ABI mandated alignment increase, so I've introduced a new macro, DATA_ABI_ALIGNMENT, which is the ABI mandated increase only (on x86-64 I think the only one is that arrays with size 16 bytes or more (and VLAs, but that is not handled by DATA*ALIGNMENT) are at least 16 byte aligned). Bootstrapped/regtested on x86_64-linux and i686-linux. No idea about other targets, I've kept them all using DATA_ALIGNMENT, which is considered optimization increase only now, if there is some ABI mandated alignment increase on other targets, that should be done in DATA_ABI_ALIGNMENT as well as DATA_ALIGNMENT. The patch causes some vectorization regressions (tweaked in the testsuite), especially for common vars where we used to align say common arrays to 256 bits rather than the ABI mandated 128 bits, or for -fpic code, but I'm afraid we need to live with that, if you compile another file with say icc or some other compiler which doesn't increase alignment beyond ABI mandated level and that other file defines the var say as non-common, we have wrong-code. 2013-06-07 Jakub Jelinek ja...@redhat.com PR target/56564 * varasm.c (align_variable): Don't use DATA_ALIGNMENT or CONSTANT_ALIGNMENT if !decl_binds_to_current_def_p (decl). Use DATA_ABI_ALIGNMENT for that case instead if defined. (get_variable_align): New function. (get_variable_section, emit_bss, emit_common, assemble_variable_contents, place_block_symbol): Use get_variable_align instead of DECL_ALIGN. (assemble_noswitch_variable): Add align argument, use it instead of DECL_ALIGN. (assemble_variable): Adjust caller. Use get_variable_align instead of DECL_ALIGN. * config/i386/i386.h (DATA_ALIGNMENT): Adjust x86_data_alignment caller. (DATA_ABI_ALIGNMENT): Define. * config/i386/i386-protos.h (x86_data_alignment): Adjust prototype. * config/i386/i386.c (x86_data_alignment): Add opt argument. If opt is false, only return the psABI mandated alignment increase. * doc/tm.texi.in (DATA_ABI_ALIGNMENT): Document. * doc/tm.texi: Regenerated. * gcc.target/i386/pr56564-1.c: New test. * gcc.target/i386/pr56564-2.c: New test. * gcc.target/i386/pr56564-3.c: New test. * gcc.target/i386/pr56564-4.c: New test. * gcc.target/i386/avx256-unaligned-load-4.c: Add -fno-common. * gcc.target/i386/avx256-unaligned-store-1.c: Likewise. * gcc.target/i386/avx256-unaligned-store-3.c: Likewise. * gcc.target/i386/avx256-unaligned-store-4.c: Likewise. * gcc.target/i386/vect-sizes-1.c: Likewise. * gcc.target/i386/memcpy-1.c: Likewise. * gcc.dg/vect/costmodel/i386/costmodel-vect-31.c (tmp): Initialize. * gcc.dg/vect/costmodel/x86_64/costmodel-vect-31.c (tmp): Likewise.
Re: FW: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Wed, Jun 19, 2013 at 11:01:59AM +0400, Igor Zamyatin wrote: The change also affects vectorizer in avx case which could be seen for gcc.dg/tree-ssa/loop-19.c test. After the change report says loop-19_bad.c:16: note: === vect_analyze_data_refs_alignment === loop-19_bad.c:16: note: vect_compute_data_ref_alignment: loop-19_bad.c:16: note: can't force alignment of ref: a[j_9] loop-19_bad.c:16: note: vect_compute_data_ref_alignment: loop-19_bad.c:16: note: can't force alignment of ref: c[j_9] AFAICS first condition in ix86_data_alignment was true before the change so 256 was a return value. Do we need to tweak this test also? I'd add -fno-common to the test. Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
Right, as you did for other cases. It works here as well. Thanks, Igor On Wed, Jun 19, 2013 at 11:05 AM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Jun 19, 2013 at 11:01:59AM +0400, Igor Zamyatin wrote: The change also affects vectorizer in avx case which could be seen for gcc.dg/tree-ssa/loop-19.c test. After the change report says loop-19_bad.c:16: note: === vect_analyze_data_refs_alignment === loop-19_bad.c:16: note: vect_compute_data_ref_alignment: loop-19_bad.c:16: note: can't force alignment of ref: a[j_9] loop-19_bad.c:16: note: vect_compute_data_ref_alignment: loop-19_bad.c:16: note: can't force alignment of ref: c[j_9] AFAICS first condition in ix86_data_alignment was true before the change so 256 was a return value. Do we need to tweak this test also? I'd add -fno-common to the test. Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Wed, Jun 19, 2013 at 11:12:21AM +0400, Igor Zamyatin wrote: Right, as you did for other cases. It works here as well. Patch preapproved. Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Wed, Jun 19, 2013 at 9:22 AM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Jun 19, 2013 at 11:12:21AM +0400, Igor Zamyatin wrote: Right, as you did for other cases. It works here as well. Patch preapproved. I wonder how much code breaks these days when we enable -fno-common by default? ... Richard.
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Wed, Jun 19, 2013 at 10:38:47AM +0200, Richard Biener wrote: On Wed, Jun 19, 2013 at 9:22 AM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Jun 19, 2013 at 11:12:21AM +0400, Igor Zamyatin wrote: Right, as you did for other cases. It works here as well. Patch preapproved. I wonder how much code breaks these days when we enable -fno-common by default? ... Somebody would need to try it ;). From vectorization POV, it surely would be better if -fno-common was the default. Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Jun 19, 2013, at 1:44 AM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Jun 19, 2013 at 10:38:47AM +0200, Richard Biener wrote: On Wed, Jun 19, 2013 at 9:22 AM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Jun 19, 2013 at 11:12:21AM +0400, Igor Zamyatin wrote: Right, as you did for other cases. It works here as well. Patch preapproved. I wonder how much code breaks these days when we enable -fno-common by default? ... Somebody would need to try it ;). Been there done that. That experiment has been running for at least 10 years now… :-)
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Jun 19, 2013, at 1:38 AM, Richard Biener richard.guent...@gmail.com wrote: On Wed, Jun 19, 2013 at 9:22 AM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Jun 19, 2013 at 11:12:21AM +0400, Igor Zamyatin wrote: Right, as you did for other cases. It works here as well. Patch preapproved. I wonder how much code breaks these days when we enable -fno-common by default? Not much. gcc as Apple shipped it, has always been no-common, and indeed the shared library scheme doesn't like common. There are a few test cases that would need -fcommon, but I don't think that is a big deal. Most oss I think is -fno-common friendly. I think gcc should default to c99, and I think c99 mode (and later) could use -fno-common by default. For pre c99 modes, I'd probably just leave it to the dust bin of history.
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
Patch preapproved. Jakub Hi, Checked into trunk: http://gcc.gnu.org/ml/gcc-cvs/2013-06/msg00646.html Thanks, K
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Thu, Jun 13, 2013 at 11:37 AM, Alan Modra amo...@gmail.com wrote: 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. The revised patch, without the DECL_P part is okay. The original code produced the necessary alignment and neither of us can find any code in public packages that increases the alignment for PPC vector types. While there is the possibility that a user could encounter an object file produced by an older GCC with less strict alignment and a version of GCC with this fix would make an incorrect assumption, this does not seem very likely in practice. Thanks, David
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 14, 2013 at 08:18:19AM +0930, Alan Modra wrote:
On Thu, Jun 13, 2013 at 05:42:17PM +0200, Jakub Jelinek wrote:
On Fri, Jun 14, 2013 at 01:07:01AM +0930, Alan Modra wrote:
@@ -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)
{
What is this code trying to do? Shouldn't it just use DECL_ALIGN
which should be set to the right value from get_variable_alignment?
I mean, if !decl_binds_to_current_def_p (decl), then using DATA_ALIGNMENT
or CONSTANT_ALIGNMENT (for anything but actually emitting the var into
object, or just as an optimization hint that very likely the decl will be
aligned enough, but not guaranteed), which are optimization, is wrong
(an ABI problem).
It is handling !DECL_P trees, which must be local. I know I saw
STRING_CST here when I wrote offsettable_ok_by_alignment, hence the
use of CONSTANT_ALIGNMENT. I'm not so sure about the need for
DATA_ALIGNMENT now, but if it was correct before then we ought to
be using both DATA_ABI_ALIGNMENT and DATA_ALIGNMENT after your
changes.
Yeah, then it makes sense. Sorry for not looking up earlier that this is
the !DECL_P case.
As for the
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
changes, that is ABI changing bugfix, so the question is, are you fine with
breaking the ABI (between 4.8 and 4.9, or if you wanted to backport it to
4.8 too (I certainly plan to backport the non-ppc DATA_ABI_ALIGNMENT changes
to 4.8.2, already am using it in our compilers))? The other option is
to fix the ABI, but keep things backwards ABI compatible. That would be
done by decreasing the alignment as it used to do before in DATA_ABI_ALIGNMENT,
and increasing it to the desirable level only in DATA_ALIGNMENT. That has
the effect that when emitting the decls into assembly e.g. the above will
now be correctly 32 byte aligned, but accesses to such decl in compiler
generated code will only assume that alignment if
decl_binds_to_current_def_p, otherwise they will keep assuming the old
(broken) lowered alignment. At least for 4.8 backport IMHO that would be a
better idea (but of course would need big comment explaning it).
Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 14, 2013 at 10:59:52AM +0200, Jakub Jelinek wrote:
On Fri, Jun 14, 2013 at 08:18:19AM +0930, Alan Modra wrote:
It is handling !DECL_P trees, which must be local. I know I saw
STRING_CST here when I wrote offsettable_ok_by_alignment, hence the
use of CONSTANT_ALIGNMENT. I'm not so sure about the need for
DATA_ALIGNMENT now, but if it was correct before then we ought to
be using both DATA_ABI_ALIGNMENT and DATA_ALIGNMENT after your
changes.
Yeah, then it makes sense. Sorry for not looking up earlier that this is
the !DECL_P case.
Your comment prodded me into looking at whether the !DECL_P code is
needed in 4.9, and it looks like we never see !DECL_P trees..
Bootstrap and regression tests all langs on powerpc64 didn't hit a
gcc_unreachable() I put there, both with -fsection-anchors and
-fno-section-anchors. David, please consider that piece of the patch
retracted.
As for the
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
changes, that is ABI changing bugfix, so the question is, are you fine with
breaking the ABI (between 4.8 and 4.9, or if you wanted to backport it to
4.8 too (I certainly plan to backport the non-ppc DATA_ABI_ALIGNMENT changes
to 4.8.2, already am using it in our compilers))? The other option is
to fix the ABI, but keep things backwards ABI compatible. That would be
done by decreasing the alignment as it used to do before in
DATA_ABI_ALIGNMENT,
and increasing it to the desirable level only in DATA_ALIGNMENT. That has
the effect that when emitting the decls into assembly e.g. the above will
now be correctly 32 byte aligned, but accesses to such decl in compiler
generated code will only assume that alignment if
decl_binds_to_current_def_p, otherwise they will keep assuming the old
(broken) lowered alignment. At least for 4.8 backport IMHO that would be a
better idea (but of course would need big comment explaning it).
I see your point, but for there to be a real problem we'd need
a) A library exporting such a type with (supposed) increased
alignment, and,
b) gcc would need to make use of the increased alignment.
(a) must be rare or non-existent or you'd think we would have had a
bug report about lack of user alignment in vector typedefs. The code
has been like this since 2001-11-07, so users have had a long time to
discover it. (Of course, this is an argument for just ignoring the
bug too.)
(b) doesn't happen in the rs6000 backend as far as I'm aware. Do you
know whether there is some optimisation based on alignment in generic
parts of gcc? A quick test like
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
long f1 (void)
{
return (long) x -32;
}
static int y __attribute__ ((aligned(32)));
long f2 (void)
{
return (long) y -32;
}
shows the -32 in both functions isn't optimised away.
--
Alan Modra
Australia Development Lab, IBM
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 14, 2013 at 08:12:02PM +0930, Alan Modra wrote:
As for the
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
changes, that is ABI changing bugfix, so the question is, are you fine with
breaking the ABI (between 4.8 and 4.9, or if you wanted to backport it to
4.8 too (I certainly plan to backport the non-ppc DATA_ABI_ALIGNMENT changes
to 4.8.2, already am using it in our compilers))? The other option is
to fix the ABI, but keep things backwards ABI compatible. That would be
done by decreasing the alignment as it used to do before in
DATA_ABI_ALIGNMENT,
and increasing it to the desirable level only in DATA_ALIGNMENT. That has
the effect that when emitting the decls into assembly e.g. the above will
now be correctly 32 byte aligned, but accesses to such decl in compiler
generated code will only assume that alignment if
decl_binds_to_current_def_p, otherwise they will keep assuming the old
(broken) lowered alignment. At least for 4.8 backport IMHO that would be a
better idea (but of course would need big comment explaning it).
I see your point, but for there to be a real problem we'd need
a) A library exporting such a type with (supposed) increased
alignment, and,
b) gcc would need to make use of the increased alignment.
(a) must be rare or non-existent or you'd think we would have had a
bug report about lack of user alignment in vector typedefs. The code
has been like this since 2001-11-07, so users have had a long time to
discover it. (Of course, this is an argument for just ignoring the
bug too.)
It doesn't have to be an exported symbol from a library, it is enough to
compile some objects using one compiler and other objects using another
compiler, then link into the same library.
(b) doesn't happen in the rs6000 backend as far as I'm aware. Do you
know whether there is some optimisation based on alignment in generic
parts of gcc? A quick test like
Tons of them, the DECL_ALIGN value is used say by get_pointer_alignment,
vectorizer assumptions, is added to MEM_ATTRS, so anything looking at
alignment in RTL can optimize too.
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
long f1 (void)
{
return (long) x -32;
}
Try (long) x 31; ? That x -32 not being optimized into x
is guess a missed optimization.
Consider if you put:
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
into one TU and compile with gcc 4.8.1, then
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
extern vec_align x;
long f1 (void)
{
return (long) x 31;
}
in another TU and compile with gcc trunk after your patch. I bet
it will be optimized into return 0; by the trunk + your patch compiler,
while the alignment will be actually just 16 byte.
Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 14, 2013 at 12:54:40PM +0200, Jakub Jelinek wrote:
On Fri, Jun 14, 2013 at 08:12:02PM +0930, Alan Modra wrote:
I see your point, but for there to be a real problem we'd need
a) A library exporting such a type with (supposed) increased
alignment, and,
b) gcc would need to make use of the increased alignment.
(a) must be rare or non-existent or you'd think we would have had a
bug report about lack of user alignment in vector typedefs. The code
has been like this since 2001-11-07, so users have had a long time to
discover it. (Of course, this is an argument for just ignoring the
bug too.)
It doesn't have to be an exported symbol from a library, it is enough to
compile some objects using one compiler and other objects using another
compiler, then link into the same library.
OK.
Try (long) x 31; ? That x -32 not being optimized into x
is guess a missed optimization.
Huh, trust me to hit another bug. :)
Consider if you put:
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
into one TU and compile with gcc 4.8.1, then
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
extern vec_align x;
long f1 (void)
{
return (long) x 31;
}
in another TU and compile with gcc trunk after your patch. I bet
it will be optimized into return 0; by the trunk + your patch compiler,
while the alignment will be actually just 16 byte.
Right. Counterpoint is that gcc made exactly the same sort of error
across TUs and even in the same TU prior to my change. eg.
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
long f1 (void) { return (long)x 31; }
int y __attribute__ ((vector_size(16), aligned(32))) = { 0, 0, 0, 0 };
long f2 (void) { return (long)y 31; }
compiles to
.L.f1:
li 3,0
blr
..
.L.f2:
li 3,0
blr
..
.globl y
.lcomm y,16,32
.type y, @object
.globl x
.lcomm x,16,16
.type x, @object
.ident GCC: (GNU) 4.7.2 20120921 (Red Hat 4.7.2-2)
My implementation of rs6000_data_alignment therefore doesn't introduce
a *new* ABI incompatibility. I question whether it is worth
complicating rs6000_data_alignment, especially since your suggestion
of using the older buggy alignment in DATA_ABI_ALIGNMENT then
increasing in DATA_ALIGNMENT isn't as simple as it sounds. We're
not talking about some fixed increase in DATA_ALIGNMENT but what we
want is the value of alignment before DATA_ABI_ALIGNMENT. Perhaps
that could be retrieved from TYPE_ALIGN (type) and
MAX_OFILE_ALIGNMENT, but that would make our DATA_ALIGNMENT the only
target to need such tricks.
--
Alan Modra
Australia Development Lab, IBM
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
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.)
* config/rs6000/rs6000.h (DATA_ABI_ALIGNMENT): Define.
(DATA_ALIGNMENT): Remove alignment already covered by above.
(LOCAL_ALIGNMENT): Use both DATA_ABI_ALIGNMENT and DATA_ALIGNMENT.
Index: gcc/config/rs6000/rs6000.h
===
--- gcc/config/rs6000/rs6000.h (revision 200055)
+++ gcc/config/rs6000/rs6000.h (working copy)
@@ -817,7 +817,8 @@ extern unsigned rs6000_pointer_size;
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)
+ ({unsigned int _align = DATA_ABI_ALIGNMENT (TYPE, ALIGN);\
+DATA_ALIGNMENT (TYPE, _align); })
/* Alignment of field after `int : 0' in a structure. */
#define EMPTY_FIELD_BOUNDARY 32
@@ -837,21 +838,26 @@ 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)\
+ ((TREE_CODE (TYPE) == ARRAY_TYPE \
+ TYPE_MODE (TREE_TYPE (TYPE)) == QImode \
+ (ALIGN) BITS_PER_WORD) ? BITS_PER_WORD : (ALIGN))
+
+/* Align vectors to 128 bits. Align SPE vectors and E500 v2 doubles to
64 bits. */
-#define DATA_ALIGNMENT(TYPE, ALIGN)\
+#define DATA_ABI_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)))
+ ? ((ALIGN) 64 ? 64 : (ALIGN)) \
+ : ((ALIGN) 128 ? 128 : (ALIGN))) \
+ : (TARGET_E500_DOUBLE \
+ TREE_CODE (TYPE) == REAL_TYPE \
+ TYPE_MODE (TYPE) == DFmode\
+ (ALIGN) 64) \
+ ? 64
\
+ : (ALIGN))
/* Nonzero if move instructions will actually fail to work
when given unaligned data. */
--
Alan Modra
Australia Development Lab, IBM
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
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)
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 14, 2013 at 01:07:01AM +0930, Alan Modra wrote:
@@ -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)
{
What is this code trying to do? Shouldn't it just use DECL_ALIGN
which should be set to the right value from get_variable_alignment?
I mean, if !decl_binds_to_current_def_p (decl), then using DATA_ALIGNMENT
or CONSTANT_ALIGNMENT (for anything but actually emitting the var into
object, or just as an optimization hint that very likely the decl will be
aligned enough, but not guaranteed), which are optimization, is wrong
(an ABI problem).
Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Thu, Jun 13, 2013 at 05:42:17PM +0200, Jakub Jelinek wrote:
On Fri, Jun 14, 2013 at 01:07:01AM +0930, Alan Modra wrote:
@@ -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)
{
What is this code trying to do? Shouldn't it just use DECL_ALIGN
which should be set to the right value from get_variable_alignment?
I mean, if !decl_binds_to_current_def_p (decl), then using DATA_ALIGNMENT
or CONSTANT_ALIGNMENT (for anything but actually emitting the var into
object, or just as an optimization hint that very likely the decl will be
aligned enough, but not guaranteed), which are optimization, is wrong
(an ABI problem).
It is handling !DECL_P trees, which must be local. I know I saw
STRING_CST here when I wrote offsettable_ok_by_alignment, hence the
use of CONSTANT_ALIGNMENT. I'm not so sure about the need for
DATA_ALIGNMENT now, but if it was correct before then we ought to
be using both DATA_ABI_ALIGNMENT and DATA_ALIGNMENT after your
changes.
--
Alan Modra
Australia Development Lab, IBM
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
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.
Edmar
On Fri, Jun 7, 2013 at 3:43 PM, Richard Henderson r...@redhat.com wrote:
On 06/07/2013 12:25 PM, Jakub Jelinek wrote:
This PR is about DATA_ALIGNMENT macro increasing alignment of some decls
for optimization purposes beyond ABI mandated levels. It is fine to emit
the vars aligned as much as we want for optimization purposes, but if we
can't be sure that references to that decl bind to the definition we
increased the alignment on (e.g. common variables, or -fpic code without
hidden visibility, weak vars etc.), we can't assume that alignment.
When the linker merges common blocks, it chooses both maximum size and maximum
alignment. Thus for any common block for which we can prove the block must
reside in the module (any executable, or hidden common in shared object), we
can go ahead and use the increased alignment.
It's only in shared objects with non-hidden common blocks that we have a
problem, since in that case we may resolve the common block via copy reloc to
a
memory block in another module.
So while decl_binds_to_current_def_p is a good starting point, I think we can
do a little better with common blocks. Which ought to take care of those
vectorization regressions you mention.
@@ -966,8 +966,12 @@ align_variable (tree decl, bool dont_out
align = MAX_OFILE_ALIGNMENT;
}
- /* On some machines, it is good to increase alignment sometimes. */
- if (! DECL_USER_ALIGN (decl))
+ /* On some machines, it is good to increase alignment sometimes.
+ But as DECL_ALIGN is used both for actually emitting the variable
+ and for code accessing the variable as guaranteed alignment, we
+ can only increase the alignment if it is a performance optimization
+ if the references to it must bind to the current definition. */
+ if (! DECL_USER_ALIGN (decl) decl_binds_to_current_def_p (decl))
{
#ifdef DATA_ALIGNMENT
unsigned int data_align = DATA_ALIGNMENT (TREE_TYPE (decl), align);
@@ -988,12 +992,69 @@ align_variable (tree decl, bool dont_out
}
#endif
}
+#ifdef DATA_ABI_ALIGNMENT
+ else if (! DECL_USER_ALIGN (decl))
+{
+ unsigned int data_align = DATA_ABI_ALIGNMENT (TREE_TYPE (decl),
align);
+ /* For backwards compatibility, don't assume the ABI alignment for
+ TLS variables. */
+ if (! DECL_THREAD_LOCAL_P (decl) || data_align = BITS_PER_WORD)
+ align = data_align;
+}
+#endif
This structure would seem to do the wrong thing if DATA_ABI_ALIGNMENT is
defined, but DATA_ALIGNMENT isn't. And while I realize you documented it, I
don't like the restriction that D_A /must/ return something larger than D_A_A.
All that means is that in complex cases D_A will have to call D_A_A itself.
I would think that it would be better to rearrange as
if (!D_U_A)
{
#ifdef D_A_A
align = ...
#endif
#ifdef D_A
if (d_b_t_c_d_p)
align = ...
#endif
}
Why the special case for TLS? If we really want that special case surely that
test should go into D_A_A itself, and not here in generic code.
Bootstrapped/regtested on x86_64-linux and i686-linux. No idea about other
targets, I've kept them all using DATA_ALIGNMENT, which is considered
optimization increase only now, if there is some ABI mandated alignment
increase on other targets, that should be done in DATA_ABI_ALIGNMENT as
well as DATA_ALIGNMENT.
I've had a brief look over the instances of D_A within the tree atm. Most of
them carry the cut-n-paste comment for the same reasons. These I believe
never intended an ABI change, and were really only interested in optimization.
But these I think require a good hard look to see if they really intended an
ABI alignment:
c6x comment explicitly mentions abi
criscompiler options for alignment -- systemwide or local?
mmixcomment mentions GETA instruction
s390comment mentions LARL instruction
rs6000 SPE and E500 portion of the alignment non-optional?
Relevant port maintainers CCed.
r~
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Mon, Jun 10, 2013 at 08:44:05PM -0400, DJ Delorie wrote: @@ -986,12 +1053,10 @@ align_variable (tree decl, bool dont_out if (! DECL_THREAD_LOCAL_P (decl) || const_align = BITS_PER_WORD) align = const_align; } -#endif } +#endif I think this change in get_variable_align() is wrong; it results in unbalanced braces inside an #ifdef, if the #ifdef body is not included (i.e. CONSTANT_ALIGNMENT not defined), the compile fails... You are right, fixed thusly, committed as obvious: 2013-06-11 Jakub Jelinek ja...@redhat.com PR target/56564 * varasm.c (get_variable_align): Move #endif to the right place. --- gcc/varasm.c(revision 199933) +++ gcc/varasm.c(revision 199934) @@ -1053,8 +1053,8 @@ get_variable_align (tree decl) if (! DECL_THREAD_LOCAL_P (decl) || const_align = BITS_PER_WORD) align = const_align; } -} #endif +} return align; } Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
Thanks!
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On 06/07/2013 10:43 PM, Richard Henderson wrote: But these I think require a good hard look to see if they really intended an ABI alignment: c6x comment explicitly mentions abi The ABI specifies a minimum alignment for arrays. Bernd
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Mon, Jun 10, 2013 at 12:51:05PM +0200, Bernd Schmidt wrote: On 06/07/2013 10:43 PM, Richard Henderson wrote: But these I think require a good hard look to see if they really intended an ABI alignment: c6x comment explicitly mentions abi The ABI specifies a minimum alignment for arrays. Thus after the patch c6x.h (DATA_ALIGNMENT) should be renamed to DATA_ABI_ALIGNMENT, right? Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On 06/10/2013 12:55 PM, Jakub Jelinek wrote: On Mon, Jun 10, 2013 at 12:51:05PM +0200, Bernd Schmidt wrote: On 06/07/2013 10:43 PM, Richard Henderson wrote: But these I think require a good hard look to see if they really intended an ABI alignment: c6x comment explicitly mentions abi The ABI specifies a minimum alignment for arrays. Thus after the patch c6x.h (DATA_ALIGNMENT) should be renamed to DATA_ABI_ALIGNMENT, right? I think so. Bernd
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
Richard Henderson wrote: s390 comment mentions LARL instruction On s390(x) it is indeed an ABI requirement that all global symbols are at least 2-aligned. (Note that we skip that alignment requirement if a symbol is marked as attribute((aligned(1)), but that attribute must then be present for every use, too.) Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE ulrich.weig...@de.ibm.com
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On 06/07/2013 02:14 PM, Jakub Jelinek wrote:
When the linker merges common blocks, it chooses both maximum size and
maximum
alignment. Thus for any common block for which we can prove the block must
reside in the module (any executable, or hidden common in shared object),
we
can go ahead and use the increased alignment.
But consider say:
one TU:
struct S { char buf[15]; } s __attribute__((aligned (32)));
another TU:
char c = 7;
struct S { char buf[15]; } s = { { 1, 2 } };
char d = 8;
int main () { return 0; }
(the aligned(32) is there just to simulate the DATA_ALIGNMENT optimization
increase). Linker warns about this (thus the question is if we want to
increase the alignment for optimization on commons at all) and doesn't align
it.
Oh, right. I hadn't considered commons unifying with non-common variables,
and the failure of commoning in that case. I'd mostly been thinking of
uninitialized Fortran-like common blocks, where it is more common for the
blocks to have nothing in common but the name.
r~
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Mon, Jun 10, 2013 at 07:51:54AM -0700, Richard Henderson wrote:
On 06/07/2013 02:14 PM, Jakub Jelinek wrote:
When the linker merges common blocks, it chooses both maximum size and
maximum
alignment. Thus for any common block for which we can prove the block
must
reside in the module (any executable, or hidden common in shared
object), we
can go ahead and use the increased alignment.
But consider say:
one TU:
struct S { char buf[15]; } s __attribute__((aligned (32)));
another TU:
char c = 7;
struct S { char buf[15]; } s = { { 1, 2 } };
char d = 8;
int main () { return 0; }
(the aligned(32) is there just to simulate the DATA_ALIGNMENT optimization
increase). Linker warns about this (thus the question is if we want to
increase the alignment for optimization on commons at all) and doesn't align
it.
Oh, right. I hadn't considered commons unifying with non-common variables,
and the failure of commoning in that case. I'd mostly been thinking of
uninitialized Fortran-like common blocks, where it is more common for the
blocks to have nothing in common but the name.
Ok, here is what I've committed to trunk (will wait for some time before
backporting). As discussed with Honza on IRC, decl_binds_to_current_def_p
will need further fixing, it does the wrong thing for extern int var
__attribute__((visibility (hidden))) or hidden DECL_COMMON symbols.
And, we don't have any feedback about SPE/E500 rs6000 - DATA_ALIGNMENT vs.
DATA_ABI_ALIGNMENT yet.
2013-06-10 Jakub Jelinek ja...@redhat.com
PR target/56564
* varasm.c (align_variable): Don't use DATA_ALIGNMENT or
CONSTANT_ALIGNMENT if !decl_binds_to_current_def_p (decl).
Use DATA_ABI_ALIGNMENT for that case instead if defined.
(get_variable_align): New function.
(get_variable_section, emit_bss, emit_common,
assemble_variable_contents, place_block_symbol): Use
get_variable_align instead of DECL_ALIGN.
(assemble_noswitch_variable): Add align argument, use it
instead of DECL_ALIGN.
(assemble_variable): Adjust caller. Use get_variable_align
instead of DECL_ALIGN.
* config/i386/i386.h (DATA_ALIGNMENT): Adjust x86_data_alignment
caller.
(DATA_ABI_ALIGNMENT): Define.
* config/i386/i386-protos.h (x86_data_alignment): Adjust prototype.
* config/i386/i386.c (x86_data_alignment): Add opt argument. If
opt is false, only return the psABI mandated alignment increase.
* config/c6x/c6x.h (DATA_ALIGNMENT): Renamed to...
(DATA_ABI_ALIGNMENT): ... this.
* config/mmix/mmix.h (DATA_ALIGNMENT): Renamed to...
(DATA_ABI_ALIGNMENT): ... this.
* config/mmix/mmix.c (mmix_data_alignment): Adjust function comment.
* config/s390/s390.h (DATA_ALIGNMENT): Renamed to...
(DATA_ABI_ALIGNMENT): ... this.
* doc/tm.texi.in (DATA_ABI_ALIGNMENT): Document.
* doc/tm.texi: Regenerated.
* gcc.target/i386/pr56564-1.c: New test.
* gcc.target/i386/pr56564-2.c: New test.
* gcc.target/i386/pr56564-3.c: New test.
* gcc.target/i386/pr56564-4.c: New test.
* gcc.target/i386/avx256-unaligned-load-4.c: Add -fno-common.
* gcc.target/i386/avx256-unaligned-store-1.c: Likewise.
* gcc.target/i386/avx256-unaligned-store-3.c: Likewise.
* gcc.target/i386/avx256-unaligned-store-4.c: Likewise.
* gcc.target/i386/vect-sizes-1.c: Likewise.
* gcc.target/i386/memcpy-1.c: Likewise.
* gcc.dg/vect/costmodel/i386/costmodel-vect-31.c (tmp): Initialize.
* gcc.dg/vect/costmodel/x86_64/costmodel-vect-31.c (tmp): Likewise.
--- gcc/config/c6x/c6x.h.jj 2013-02-26 16:39:34.0 +0100
+++ gcc/config/c6x/c6x.h2013-06-10 17:36:44.850082918 +0200
@@ -134,7 +134,7 @@ extern c6x_cpu_t c6x_arch;
Really only externally visible arrays must be aligned this way, as
only those are directly visible from another compilation unit. But
we don't have that information available here. */
-#define DATA_ALIGNMENT(TYPE, ALIGN)\
+#define DATA_ABI_ALIGNMENT(TYPE, ALIGN)
\
(((ALIGN) BITS_PER_UNIT * 8 TREE_CODE (TYPE) == ARRAY_TYPE) \
? BITS_PER_UNIT * 8 : (ALIGN))
--- gcc/config/mmix/mmix.h.jj 2013-01-11 09:03:16.0 +0100
+++ gcc/config/mmix/mmix.h 2013-06-10 17:36:05.585730695 +0200
@@ -164,7 +164,7 @@ struct GTY(()) machine_function
/* Copied from elfos.h. */
#define MAX_OFILE_ALIGNMENT (32768 * 8)
-#define DATA_ALIGNMENT(TYPE, BASIC_ALIGN) \
+#define DATA_ABI_ALIGNMENT(TYPE, BASIC_ALIGN) \
mmix_data_alignment (TYPE, BASIC_ALIGN)
#define CONSTANT_ALIGNMENT(CONSTANT, BASIC_ALIGN) \
--- gcc/config/mmix/mmix.c.jj 2013-03-26 10:03:58.0 +0100
+++ gcc/config/mmix/mmix.c 2013-06-10 17:36:28.012360493 +0200
@@ -313,7 +313,7 @@
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Mon, Jun 10, 2013 at 11:44 AM, Jakub Jelinek ja...@redhat.com wrote:
On Mon, Jun 10, 2013 at 07:51:54AM -0700, Richard Henderson wrote:
On 06/07/2013 02:14 PM, Jakub Jelinek wrote:
When the linker merges common blocks, it chooses both maximum size and
maximum
alignment. Thus for any common block for which we can prove the block
must
reside in the module (any executable, or hidden common in shared
object), we
can go ahead and use the increased alignment.
But consider say:
one TU:
struct S { char buf[15]; } s __attribute__((aligned (32)));
another TU:
char c = 7;
struct S { char buf[15]; } s = { { 1, 2 } };
char d = 8;
int main () { return 0; }
(the aligned(32) is there just to simulate the DATA_ALIGNMENT optimization
increase). Linker warns about this (thus the question is if we want to
increase the alignment for optimization on commons at all) and doesn't
align
it.
Oh, right. I hadn't considered commons unifying with non-common variables,
and the failure of commoning in that case. I'd mostly been thinking of
uninitialized Fortran-like common blocks, where it is more common for the
blocks to have nothing in common but the name.
Ok, here is what I've committed to trunk (will wait for some time before
backporting). As discussed with Honza on IRC, decl_binds_to_current_def_p
will need further fixing, it does the wrong thing for extern int var
__attribute__((visibility (hidden))) or hidden DECL_COMMON symbols.
And, we don't have any feedback about SPE/E500 rs6000 - DATA_ALIGNMENT vs.
DATA_ABI_ALIGNMENT yet.
SPE/e500 support was written by Aldy. He or someone from Freescale
needs to comment.
- David
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
@@ -986,12 +1053,10 @@ align_variable (tree decl, bool dont_out if (! DECL_THREAD_LOCAL_P (decl) || const_align = BITS_PER_WORD) align = const_align; } -#endif } +#endif I think this change in get_variable_align() is wrong; it results in unbalanced braces inside an #ifdef, if the #ifdef body is not included (i.e. CONSTANT_ALIGNMENT not defined), the compile fails...
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 07, 2013 at 06:56:34PM -0400, Hans-Peter Nilsson wrote: criscompiler options for alignment -- systemwide or local? No, DATA_ALIGNMENT in cris.h is not intended as an ABI indication, but as an optimization when emitting data. (This was the way to do it at the time. Has this changed?) The ABI is as indicated by BIGGEST_ALIGNMENT: 8 (bits; one byte). Nothing is guaranteed (to the data referer) to have a bigger alignment - unless otherwise indicated by attribute-align. (Unfortunately I can't change BIGGEST_ALIGNMENT to indicate that atomic variables require natural alignment, or actually not to straddle a cache-boundary, as increasing BIGGEST_ALIGNMENT makes GCC change the ABI. But that's a slightly different issue.) Right now it is unfortunately part of ABI, which is something the patch attempts to cure in a backwards compatible way (i.e., data will be still alignment the way it used to be, but code will no longer assume it is that much aligned, unless it is DATA_ABI_ALIGNMENT). mmixcomment mentions GETA instruction Yep, data must be at least 32-bit-aligned so addresses can be formed with a GETA insn. BIGGEST_ALIGNMENT is 64 though and STRICT_ALIGNMENT; natural alignment is required for proper interpretation as the low bits are ignored. Then mmix would probably want to define DATA_ABI_ALIGNMENT instead of DATA_ALIGNMENT after the patch (or both). Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 07, 2013 at 11:14:19PM +0200, Jakub Jelinek wrote:
This structure would seem to do the wrong thing if DATA_ABI_ALIGNMENT is
defined, but DATA_ALIGNMENT isn't. And while I realize you documented it, I
don't like the restriction that D_A /must/ return something larger than
D_A_A.
All that means is that in complex cases D_A will have to call D_A_A itself.
Yeah, I guess I can rearrange it. The reason I wrote it that way was to
avoid an extra function call, but that is probably not big enough overhead.
Here is the code rearranged so that DATA_ABI_ALIGNMENT is independent of
DATA_ALIGNMENT. The rest of stuff is kept as is.
As for the commons getting bigger alignment than the ABI has for them, I'm
afraid the linker usually has no option but to warn and don't do anything.
Because if the non-common definition that is supposed to win over the common
one isn't sufficiently aligned (and it could be aligned just to the ABI
mandated boundary), that definition could be already in the middle of say
.data or other section and so the linker doesn't have the luxury of aligning
it individually.
E.g. vect_can_force_dr_alignment_p has been changed some time ago to:
/* We cannot change alignment of common or external symbols as another
translation unit may contain a definition with lower alignment.
The rules of common symbol linking mean that the definition
will override the common symbol. The same is true for constant
pool entries which may be shared and are not properly merged
by LTO. */
if (DECL_EXTERNAL (decl)
|| DECL_COMMON (decl)
|| DECL_IN_CONSTANT_POOL (decl))
return false;
but at that point we haven't changed align_variable. Thus perhaps we want
in align_variable handle DECL_COMMON the same way as we handle TLS with
word alignment.
2013-06-08 Jakub Jelinek ja...@redhat.com
PR target/56564
* varasm.c (align_variable): Don't use DATA_ALIGNMENT or
CONSTANT_ALIGNMENT if !decl_binds_to_current_def_p (decl).
Use DATA_ABI_ALIGNMENT for that case instead if defined.
(get_variable_align): New function.
(get_variable_section, emit_bss, emit_common,
assemble_variable_contents, place_block_symbol): Use
get_variable_align instead of DECL_ALIGN.
(assemble_noswitch_variable): Add align argument, use it
instead of DECL_ALIGN.
(assemble_variable): Adjust caller. Use get_variable_align
instead of DECL_ALIGN.
* config/i386/i386.h (DATA_ALIGNMENT): Adjust x86_data_alignment
caller.
(DATA_ABI_ALIGNMENT): Define.
* config/i386/i386-protos.h (x86_data_alignment): Adjust prototype.
* config/i386/i386.c (x86_data_alignment): Add opt argument. If
opt is false, only return the psABI mandated alignment increase.
* doc/tm.texi.in (DATA_ABI_ALIGNMENT): Document.
* doc/tm.texi: Regenerated.
* gcc.target/i386/pr56564-1.c: New test.
* gcc.target/i386/pr56564-2.c: New test.
* gcc.target/i386/pr56564-3.c: New test.
* gcc.target/i386/pr56564-4.c: New test.
* gcc.target/i386/avx256-unaligned-load-4.c: Add -fno-common.
* gcc.target/i386/avx256-unaligned-store-1.c: Likewise.
* gcc.target/i386/avx256-unaligned-store-3.c: Likewise.
* gcc.target/i386/avx256-unaligned-store-4.c: Likewise.
* gcc.target/i386/vect-sizes-1.c: Likewise.
* gcc.target/i386/memcpy-1.c: Likewise.
* gcc.dg/vect/costmodel/i386/costmodel-vect-31.c (tmp): Initialize.
* gcc.dg/vect/costmodel/x86_64/costmodel-vect-31.c (tmp): Likewise.
--- gcc/varasm.c.jj 2013-06-07 13:17:17.0 +0200
+++ gcc/varasm.c2013-06-08 16:53:40.717372488 +0200
@@ -966,13 +966,80 @@ align_variable (tree decl, bool dont_out
align = MAX_OFILE_ALIGNMENT;
}
- /* On some machines, it is good to increase alignment sometimes. */
if (! DECL_USER_ALIGN (decl))
{
+#ifdef DATA_ABI_ALIGNMENT
+ unsigned int data_abi_align
+ = DATA_ABI_ALIGNMENT (TREE_TYPE (decl), align);
+ /* For backwards compatibility, don't assume the ABI alignment for
+TLS variables. */
+ if (! DECL_THREAD_LOCAL_P (decl) || data_abi_align = BITS_PER_WORD)
+ align = data_abi_align;
+#endif
+
+ /* On some machines, it is good to increase alignment sometimes.
+But as DECL_ALIGN is used both for actually emitting the variable
+and for code accessing the variable as guaranteed alignment, we
+can only increase the alignment if it is a performance optimization
+if the references to it must bind to the current definition. */
+ if (decl_binds_to_current_def_p (decl))
+ {
+#ifdef DATA_ALIGNMENT
+ unsigned int data_align = DATA_ALIGNMENT (TREE_TYPE (decl), align);
+ /* Don't increase alignment too much for TLS variables - TLS space
+is too precious. */
+ if (!
[PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
Hi!
This PR is about DATA_ALIGNMENT macro increasing alignment of some decls
for optimization purposes beyond ABI mandated levels. It is fine to emit
the vars aligned as much as we want for optimization purposes, but if we
can't be sure that references to that decl bind to the definition we
increased the alignment on (e.g. common variables, or -fpic code without
hidden visibility, weak vars etc.), we can't assume that alignment.
As DECL_ALIGN is used for both the alignment emitted for the definitions
and alignment assumed on code referring to it, this patch increases
DECL_ALIGN only on decls where decl_binds_to_current_def_p is true,
and otherwise the optimization part on top of that emits only when
aligning definition.
On x86_64, DATA_ALIGNMENT macro was partly an optimization, partly ABI
mandated alignment increase, so I've introduced a new macro,
DATA_ABI_ALIGNMENT, which is the ABI mandated increase only (on x86-64
I think the only one is that arrays with size 16 bytes or more (and VLAs,
but that is not handled by DATA*ALIGNMENT) are at least 16 byte aligned).
Bootstrapped/regtested on x86_64-linux and i686-linux. No idea about other
targets, I've kept them all using DATA_ALIGNMENT, which is considered
optimization increase only now, if there is some ABI mandated alignment
increase on other targets, that should be done in DATA_ABI_ALIGNMENT as
well as DATA_ALIGNMENT. The patch causes some vectorization regressions
(tweaked in the testsuite), especially for common vars where we used to
align say common arrays to 256 bits rather than the ABI mandated 128 bits,
or for -fpic code, but I'm afraid we need to live with that, if you compile
another file with say icc or some other compiler which doesn't increase
alignment beyond ABI mandated level and that other file defines the var say
as non-common, we have wrong-code.
2013-06-07 Jakub Jelinek ja...@redhat.com
PR target/56564
* varasm.c (align_variable): Don't use DATA_ALIGNMENT or
CONSTANT_ALIGNMENT if !decl_binds_to_current_def_p (decl).
Use DATA_ABI_ALIGNMENT for that case instead if defined.
(get_variable_align): New function.
(get_variable_section, emit_bss, emit_common,
assemble_variable_contents, place_block_symbol): Use
get_variable_align instead of DECL_ALIGN.
(assemble_noswitch_variable): Add align argument, use it
instead of DECL_ALIGN.
(assemble_variable): Adjust caller. Use get_variable_align
instead of DECL_ALIGN.
* config/i386/i386.h (DATA_ALIGNMENT): Adjust x86_data_alignment
caller.
(DATA_ABI_ALIGNMENT): Define.
* config/i386/i386-protos.h (x86_data_alignment): Adjust prototype.
* config/i386/i386.c (x86_data_alignment): Add opt argument. If
opt is false, only return the psABI mandated alignment increase.
* doc/tm.texi.in (DATA_ABI_ALIGNMENT): Document.
* doc/tm.texi: Regenerated.
* gcc.target/i386/pr56564-1.c: New test.
* gcc.target/i386/pr56564-2.c: New test.
* gcc.target/i386/pr56564-3.c: New test.
* gcc.target/i386/pr56564-4.c: New test.
* gcc.target/i386/avx256-unaligned-load-4.c: Add -fno-common.
* gcc.target/i386/avx256-unaligned-store-1.c: Likewise.
* gcc.target/i386/avx256-unaligned-store-3.c: Likewise.
* gcc.target/i386/avx256-unaligned-store-4.c: Likewise.
* gcc.target/i386/vect-sizes-1.c: Likewise.
* gcc.target/i386/memcpy-1.c: Likewise.
* gcc.dg/vect/costmodel/i386/costmodel-vect-31.c (tmp): Initialize.
* gcc.dg/vect/costmodel/x86_64/costmodel-vect-31.c (tmp): Likewise.
--- gcc/varasm.c.jj 2013-06-07 13:17:17.0 +0200
+++ gcc/varasm.c2013-06-07 15:38:36.710908852 +0200
@@ -966,8 +966,12 @@ align_variable (tree decl, bool dont_out
align = MAX_OFILE_ALIGNMENT;
}
- /* On some machines, it is good to increase alignment sometimes. */
- if (! DECL_USER_ALIGN (decl))
+ /* On some machines, it is good to increase alignment sometimes.
+ But as DECL_ALIGN is used both for actually emitting the variable
+ and for code accessing the variable as guaranteed alignment, we
+ can only increase the alignment if it is a performance optimization
+ if the references to it must bind to the current definition. */
+ if (! DECL_USER_ALIGN (decl) decl_binds_to_current_def_p (decl))
{
#ifdef DATA_ALIGNMENT
unsigned int data_align = DATA_ALIGNMENT (TREE_TYPE (decl), align);
@@ -988,12 +992,69 @@ align_variable (tree decl, bool dont_out
}
#endif
}
+#ifdef DATA_ABI_ALIGNMENT
+ else if (! DECL_USER_ALIGN (decl))
+{
+ unsigned int data_align = DATA_ABI_ALIGNMENT (TREE_TYPE (decl), align);
+ /* For backwards compatibility, don't assume the ABI alignment for
+TLS variables. */
+ if (! DECL_THREAD_LOCAL_P (decl) || data_align = BITS_PER_WORD)
+ align = data_align;
+}
+#endif
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On 06/07/2013 12:25 PM, Jakub Jelinek wrote:
This PR is about DATA_ALIGNMENT macro increasing alignment of some decls
for optimization purposes beyond ABI mandated levels. It is fine to emit
the vars aligned as much as we want for optimization purposes, but if we
can't be sure that references to that decl bind to the definition we
increased the alignment on (e.g. common variables, or -fpic code without
hidden visibility, weak vars etc.), we can't assume that alignment.
When the linker merges common blocks, it chooses both maximum size and maximum
alignment. Thus for any common block for which we can prove the block must
reside in the module (any executable, or hidden common in shared object), we
can go ahead and use the increased alignment.
It's only in shared objects with non-hidden common blocks that we have a
problem, since in that case we may resolve the common block via copy reloc to a
memory block in another module.
So while decl_binds_to_current_def_p is a good starting point, I think we can
do a little better with common blocks. Which ought to take care of those
vectorization regressions you mention.
@@ -966,8 +966,12 @@ align_variable (tree decl, bool dont_out
align = MAX_OFILE_ALIGNMENT;
}
- /* On some machines, it is good to increase alignment sometimes. */
- if (! DECL_USER_ALIGN (decl))
+ /* On some machines, it is good to increase alignment sometimes.
+ But as DECL_ALIGN is used both for actually emitting the variable
+ and for code accessing the variable as guaranteed alignment, we
+ can only increase the alignment if it is a performance optimization
+ if the references to it must bind to the current definition. */
+ if (! DECL_USER_ALIGN (decl) decl_binds_to_current_def_p (decl))
{
#ifdef DATA_ALIGNMENT
unsigned int data_align = DATA_ALIGNMENT (TREE_TYPE (decl), align);
@@ -988,12 +992,69 @@ align_variable (tree decl, bool dont_out
}
#endif
}
+#ifdef DATA_ABI_ALIGNMENT
+ else if (! DECL_USER_ALIGN (decl))
+{
+ unsigned int data_align = DATA_ABI_ALIGNMENT (TREE_TYPE (decl), align);
+ /* For backwards compatibility, don't assume the ABI alignment for
+ TLS variables. */
+ if (! DECL_THREAD_LOCAL_P (decl) || data_align = BITS_PER_WORD)
+ align = data_align;
+}
+#endif
This structure would seem to do the wrong thing if DATA_ABI_ALIGNMENT is
defined, but DATA_ALIGNMENT isn't. And while I realize you documented it, I
don't like the restriction that D_A /must/ return something larger than D_A_A.
All that means is that in complex cases D_A will have to call D_A_A itself.
I would think that it would be better to rearrange as
if (!D_U_A)
{
#ifdef D_A_A
align = ...
#endif
#ifdef D_A
if (d_b_t_c_d_p)
align = ...
#endif
}
Why the special case for TLS? If we really want that special case surely that
test should go into D_A_A itself, and not here in generic code.
Bootstrapped/regtested on x86_64-linux and i686-linux. No idea about other
targets, I've kept them all using DATA_ALIGNMENT, which is considered
optimization increase only now, if there is some ABI mandated alignment
increase on other targets, that should be done in DATA_ABI_ALIGNMENT as
well as DATA_ALIGNMENT.
I've had a brief look over the instances of D_A within the tree atm. Most of
them carry the cut-n-paste comment for the same reasons. These I believe
never intended an ABI change, and were really only interested in optimization.
But these I think require a good hard look to see if they really intended an
ABI alignment:
c6x comment explicitly mentions abi
criscompiler options for alignment -- systemwide or local?
mmixcomment mentions GETA instruction
s390comment mentions LARL instruction
rs6000 SPE and E500 portion of the alignment non-optional?
Relevant port maintainers CCed.
r~
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, Jun 07, 2013 at 01:43:27PM -0700, Richard Henderson wrote:
On 06/07/2013 12:25 PM, Jakub Jelinek wrote:
This PR is about DATA_ALIGNMENT macro increasing alignment of some decls
for optimization purposes beyond ABI mandated levels. It is fine to emit
the vars aligned as much as we want for optimization purposes, but if we
can't be sure that references to that decl bind to the definition we
increased the alignment on (e.g. common variables, or -fpic code without
hidden visibility, weak vars etc.), we can't assume that alignment.
When the linker merges common blocks, it chooses both maximum size and maximum
alignment. Thus for any common block for which we can prove the block must
reside in the module (any executable, or hidden common in shared object), we
can go ahead and use the increased alignment.
But consider say:
one TU:
struct S { char buf[15]; } s __attribute__((aligned (32)));
another TU:
char c = 7;
struct S { char buf[15]; } s = { { 1, 2 } };
char d = 8;
int main () { return 0; }
(the aligned(32) is there just to simulate the DATA_ALIGNMENT optimization
increase). Linker warns about this (thus the question is if we want to
increase the alignment for optimization on commons at all) and doesn't align
it.
This structure would seem to do the wrong thing if DATA_ABI_ALIGNMENT is
defined, but DATA_ALIGNMENT isn't. And while I realize you documented it, I
don't like the restriction that D_A /must/ return something larger than D_A_A.
All that means is that in complex cases D_A will have to call D_A_A itself.
Yeah, I guess I can rearrange it. The reason I wrote it that way was to
avoid an extra function call, but that is probably not big enough overhead.
I would think that it would be better to rearrange as
if (!D_U_A)
{
#ifdef D_A_A
align = ...
#endif
#ifdef D_A
if (d_b_t_c_d_p)
align = ...
#endif
}
Why the special case for TLS? If we really want that special case surely that
test should go into D_A_A itself, and not here in generic code.
I special case TLS for backwards ABI compatibility with GCC = 4.8.1,
because we ignored DATA_ALIGNMENT for TLS vars, even if what it returned
wasn't just an optimization, but ABI requirement. So, if you have:
__thread char a[16] = { 1, 2 };
in one shared library and
__thread char a[16] = { 1, 2 };
use (a);
in another shared library, compile one with GCC 4.8.1 e.g. and the other one
with 4.9.0, if use (a) would assume the psABI mandated 16 byte alignment,
but the actual definition was from 4.8.1 compiled object and was only 1 byte
aligned, it could crash. Also, neither DATA_ALIGNMENT nor DATA_ABI_ALIGNMENT
sees
the decl itself, it just looks at type and previously assumed alignment.
Bootstrapped/regtested on x86_64-linux and i686-linux. No idea about other
targets, I've kept them all using DATA_ALIGNMENT, which is considered
optimization increase only now, if there is some ABI mandated alignment
increase on other targets, that should be done in DATA_ABI_ALIGNMENT as
well as DATA_ALIGNMENT.
I've had a brief look over the instances of D_A within the tree atm. Most of
them carry the cut-n-paste comment for the same reasons. These I believe
never intended an ABI change, and were really only interested in optimization.
Thanks for looking into this.
But these I think require a good hard look to see if they really intended an
ABI alignment:
c6x comment explicitly mentions abi
cris compiler options for alignment -- systemwide or local?
mmix comment mentions GETA instruction
s390 comment mentions LARL instruction
rs6000SPE and E500 portion of the alignment non-optional?
Relevant port maintainers CCed.
Jakub
Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)
On Fri, 7 Jun 2013, Richard Henderson wrote: I've had a brief look over the instances of D_A within the tree atm. Most of them carry the cut-n-paste comment for the same reasons. These I believe never intended an ABI change, and were really only interested in optimization. But these I think require a good hard look to see if they really intended an ABI alignment: I'm not sure what is about to change how? cris compiler options for alignment -- systemwide or local? No, DATA_ALIGNMENT in cris.h is not intended as an ABI indication, but as an optimization when emitting data. (This was the way to do it at the time. Has this changed?) The ABI is as indicated by BIGGEST_ALIGNMENT: 8 (bits; one byte). Nothing is guaranteed (to the data referer) to have a bigger alignment - unless otherwise indicated by attribute-align. (Unfortunately I can't change BIGGEST_ALIGNMENT to indicate that atomic variables require natural alignment, or actually not to straddle a cache-boundary, as increasing BIGGEST_ALIGNMENT makes GCC change the ABI. But that's a slightly different issue.) mmix comment mentions GETA instruction Yep, data must be at least 32-bit-aligned so addresses can be formed with a GETA insn. BIGGEST_ALIGNMENT is 64 though and STRICT_ALIGNMENT; natural alignment is required for proper interpretation as the low bits are ignored. brgds, H-P
