* [C++ patch] Reduce vtable alignment
@ 2014-05-16 18:04 Jan Hubicka
2014-05-16 19:12 ` Jan Hubicka
0 siblings, 1 reply; 7+ messages in thread
From: Jan Hubicka @ 2014-05-16 18:04 UTC (permalink / raw)
To: gcc-patches, jason
Hi,
compiling:
struct A
{
virtual void foo(void) {};
virtual void foo2(void) {};
virtual void foo3(void) {};
virtual void foo4(void) {};
virtual void foo5(void) {};
} a;
give 32 byte alignment to the virtual table on i386, because we bump up
alignments of arrays to size of vector operations. This is wasteful, since
virutal tables are never really accessed this way.
I am testing the following patch, OK if it passes?
The patch also removes apparently 20 years old hack for SPARC.
Honza
* class.c (build_vtable): Force alignment of virtual tables
to be pointer size only to save space.
Index: class.c
===================================================================
--- class.c (revision 210521)
+++ class.c (working copy)
@@ -768,11 +768,8 @@ build_vtable (tree class_type, tree name
TREE_READONLY (decl) = 1;
DECL_VIRTUAL_P (decl) = 1;
DECL_ALIGN (decl) = TARGET_VTABLE_ENTRY_ALIGN;
+ DECL_USER_ALIGN (decl) = true;
DECL_VTABLE_OR_VTT_P (decl) = 1;
- /* At one time the vtable info was grabbed 2 words at a time. This
- fails on sparc unless you have 8-byte alignment. (tiemann) */
- DECL_ALIGN (decl) = MAX (TYPE_ALIGN (double_type_node),
- DECL_ALIGN (decl));
set_linkage_according_to_type (class_type, decl);
/* The vtable has not been defined -- yet. */
DECL_EXTERNAL (decl) = 1;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C++ patch] Reduce vtable alignment
2014-05-16 18:04 [C++ patch] Reduce vtable alignment Jan Hubicka
@ 2014-05-16 19:12 ` Jan Hubicka
2014-05-19 8:52 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jan Hubicka @ 2014-05-16 19:12 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, jason
Hi,
this patch makes also the rtti type info for A in the testcase:
struct A
{
virtual void foo(void) {};
virtual void foo2(void) {};
virtual void foo3(void) {};
virtual void foo4(void) {};
virtual void foo5(void) {};
} a;
aligned only to the ABI requirement (8) instead of being bumped up to 16 bytes
by the following code in i386.c:
/* x86-64 ABI requires arrays greater than 16 bytes to be aligned
to 16byte boundary. */
if (TARGET_64BIT)
{
if ((opt ? AGGREGATE_TYPE_P (type) : TREE_CODE (type) == ARRAY_TYPE)
&& TYPE_SIZE (type)
&& TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
&& wi::geu_p (TYPE_SIZE (type), 128)
&& align < 128)
return 128;
}
Here the variable is first run through align_variable and that decides to add
optional alignment. We really want only ABI required alignment here.
Does the following patch look resonable?
* rtti.c: Include tm_p.h
(emit_tinfo_decl): Align type infos only as required by the target ABI.
Index: rtti.c
===================================================================
--- rtti.c (revision 210521)
+++ rtti.c (working copy)
@@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.
#include "coretypes.h"
#include "tm.h"
#include "tree.h"
+#include "tm_p.h"
#include "stringpool.h"
#include "stor-layout.h"
#include "cp-tree.h"
@@ -1596,6 +1597,12 @@ emit_tinfo_decl (tree decl)
DECL_INITIAL (decl) = init;
mark_used (decl);
cp_finish_decl (decl, init, false, NULL_TREE, 0);
+ /* Avoid targets optionally bumping up the alignment to improve
+ vector instruction accesses, tinfo are never accessed this way. */
+#ifdef DATA_ABI_ALIGNMENT
+ DECL_ALIGN (decl) = DATA_ABI_ALIGNMENT (decl, TYPE_ALIGN (TREE_TYPE (decl)));
+ DECL_USER_ALIGN (decl) = true;
+#endif
return true;
}
else
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C++ patch] Reduce vtable alignment
2014-05-16 19:12 ` Jan Hubicka
@ 2014-05-19 8:52 ` Richard Biener
2014-05-19 9:10 ` Jakub Jelinek
2014-05-23 17:30 ` Jan Hubicka
0 siblings, 2 replies; 7+ messages in thread
From: Richard Biener @ 2014-05-19 8:52 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Jason Merrill
On Fri, May 16, 2014 at 9:12 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> this patch makes also the rtti type info for A in the testcase:
>
> struct A
> {
> virtual void foo(void) {};
> virtual void foo2(void) {};
> virtual void foo3(void) {};
> virtual void foo4(void) {};
> virtual void foo5(void) {};
> } a;
>
> aligned only to the ABI requirement (8) instead of being bumped up to 16 bytes
> by the following code in i386.c:
> /* x86-64 ABI requires arrays greater than 16 bytes to be aligned
> to 16byte boundary. */
> if (TARGET_64BIT)
> {
> if ((opt ? AGGREGATE_TYPE_P (type) : TREE_CODE (type) == ARRAY_TYPE)
> && TYPE_SIZE (type)
> && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
> && wi::geu_p (TYPE_SIZE (type), 128)
> && align < 128)
> return 128;
> }
>
> Here the variable is first run through align_variable and that decides to add
> optional alignment. We really want only ABI required alignment here.
> Does the following patch look resonable?
Hmm, but if the optimizers or the target can rely on DATA_ABI_ALIGNMENT
then we can't really lower it. Because we can make the vtable escape
to another unit that sees it as just an array of pointers?
So this looks unsafe to me. (same may apply to the idea of
having TARGET_VTABLE_ENTRY_ALIGN at all, if that possibly
conflicts with ABI alignment requirements present otherwise).
Richard.
> * rtti.c: Include tm_p.h
> (emit_tinfo_decl): Align type infos only as required by the target ABI.
>
> Index: rtti.c
> ===================================================================
> --- rtti.c (revision 210521)
> +++ rtti.c (working copy)
> @@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.
> #include "coretypes.h"
> #include "tm.h"
> #include "tree.h"
> +#include "tm_p.h"
> #include "stringpool.h"
> #include "stor-layout.h"
> #include "cp-tree.h"
> @@ -1596,6 +1597,12 @@ emit_tinfo_decl (tree decl)
> DECL_INITIAL (decl) = init;
> mark_used (decl);
> cp_finish_decl (decl, init, false, NULL_TREE, 0);
> + /* Avoid targets optionally bumping up the alignment to improve
> + vector instruction accesses, tinfo are never accessed this way. */
> +#ifdef DATA_ABI_ALIGNMENT
> + DECL_ALIGN (decl) = DATA_ABI_ALIGNMENT (decl, TYPE_ALIGN (TREE_TYPE (decl)));
> + DECL_USER_ALIGN (decl) = true;
> +#endif
> return true;
> }
> else
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C++ patch] Reduce vtable alignment
2014-05-19 8:52 ` Richard Biener
@ 2014-05-19 9:10 ` Jakub Jelinek
2014-05-19 14:57 ` Jan Hubicka
2014-05-23 17:30 ` Jan Hubicka
1 sibling, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2014-05-19 9:10 UTC (permalink / raw)
To: Richard Biener; +Cc: Jan Hubicka, GCC Patches, Jason Merrill
On Mon, May 19, 2014 at 10:52:52AM +0200, Richard Biener wrote:
> On Fri, May 16, 2014 at 9:12 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > this patch makes also the rtti type info for A in the testcase:
> >
> > struct A
> > {
> > virtual void foo(void) {};
> > virtual void foo2(void) {};
> > virtual void foo3(void) {};
> > virtual void foo4(void) {};
> > virtual void foo5(void) {};
> > } a;
> >
> > aligned only to the ABI requirement (8) instead of being bumped up to 16 bytes
> > by the following code in i386.c:
> > /* x86-64 ABI requires arrays greater than 16 bytes to be aligned
> > to 16byte boundary. */
> > if (TARGET_64BIT)
> > {
> > if ((opt ? AGGREGATE_TYPE_P (type) : TREE_CODE (type) == ARRAY_TYPE)
> > && TYPE_SIZE (type)
> > && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
> > && wi::geu_p (TYPE_SIZE (type), 128)
> > && align < 128)
> > return 128;
> > }
> >
> > Here the variable is first run through align_variable and that decides to add
> > optional alignment. We really want only ABI required alignment here.
> > Does the following patch look resonable?
>
> Hmm, but if the optimizers or the target can rely on DATA_ABI_ALIGNMENT
> then we can't really lower it. Because we can make the vtable escape
> to another unit that sees it as just an array of pointers?
Sure, they can rely on DATA_ABI_ALIGNMENT (if that macro is defined), but
anything beyond that (such as what DATA_ALIGNMENT returns) is optimization
only. So, Honza's patch looks good for me.
> So this looks unsafe to me. (same may apply to the idea of
> having TARGET_VTABLE_ENTRY_ALIGN at all, if that possibly
> conflicts with ABI alignment requirements present otherwise).
Right now the intersection of targets overriding TARGET_VTABLE_ENTRY_ALIGN and
targets defining DATA_ABI_ALIGNMENT is empty. In any case, even in that
case one should (if DATA_ABI_ALIGNMENT is defined) apply DATA_ABI_ALIGNMENT
(on top of TARGET_VTABLE_ENTRY_ALIGN and/or TYPE_ALIGN, dunno how those two
exactly mix together) and not DATA_ALIGNMENT. But this patch is about
tinfo, not vtable.
> > * rtti.c: Include tm_p.h
> > (emit_tinfo_decl): Align type infos only as required by the target ABI.
> >
> > Index: rtti.c
> > ===================================================================
> > --- rtti.c (revision 210521)
> > +++ rtti.c (working copy)
> > @@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.
> > #include "coretypes.h"
> > #include "tm.h"
> > #include "tree.h"
> > +#include "tm_p.h"
> > #include "stringpool.h"
> > #include "stor-layout.h"
> > #include "cp-tree.h"
> > @@ -1596,6 +1597,12 @@ emit_tinfo_decl (tree decl)
> > DECL_INITIAL (decl) = init;
> > mark_used (decl);
> > cp_finish_decl (decl, init, false, NULL_TREE, 0);
> > + /* Avoid targets optionally bumping up the alignment to improve
> > + vector instruction accesses, tinfo are never accessed this way. */
> > +#ifdef DATA_ABI_ALIGNMENT
> > + DECL_ALIGN (decl) = DATA_ABI_ALIGNMENT (decl, TYPE_ALIGN (TREE_TYPE (decl)));
> > + DECL_USER_ALIGN (decl) = true;
> > +#endif
> > return true;
> > }
> > else
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C++ patch] Reduce vtable alignment
2014-05-19 9:10 ` Jakub Jelinek
@ 2014-05-19 14:57 ` Jan Hubicka
0 siblings, 0 replies; 7+ messages in thread
From: Jan Hubicka @ 2014-05-19 14:57 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, Jan Hubicka, GCC Patches, Jason Merrill
> > Hmm, but if the optimizers or the target can rely on DATA_ABI_ALIGNMENT
> > then we can't really lower it. Because we can make the vtable escape
> > to another unit that sees it as just an array of pointers?
>
> Sure, they can rely on DATA_ABI_ALIGNMENT (if that macro is defined), but
> anything beyond that (such as what DATA_ALIGNMENT returns) is optimization
> only. So, Honza's patch looks good for me.
Yep, DATA_ALIGNMENT is computed when type is finalized, so I think this should
be safe.
>
> > So this looks unsafe to me. (same may apply to the idea of
> > having TARGET_VTABLE_ENTRY_ALIGN at all, if that possibly
> > conflicts with ABI alignment requirements present otherwise).
>
> Right now the intersection of targets overriding TARGET_VTABLE_ENTRY_ALIGN and
> targets defining DATA_ABI_ALIGNMENT is empty. In any case, even in that
> case one should (if DATA_ABI_ALIGNMENT is defined) apply DATA_ABI_ALIGNMENT
> (on top of TARGET_VTABLE_ENTRY_ALIGN and/or TYPE_ALIGN, dunno how those two
> exactly mix together) and not DATA_ALIGNMENT. But this patch is about
> tinfo, not vtable.
There are two patches, one is for RTTI and other is for vtables. Vtables are
fully compiler controlled structures, as such I think we do not need to align
them as usual arrays. C++ ABI does not really speak about alignment of these,
but I believe it is safe to stop aligning them, since all we do is random
accesses at given offset of the symbol - nothing where we can use the
alignment.
We can bump it down to DATA_ALIGNMENT boundary like I do for RTTI, but it
would still waste several percent of the data segment. (Clang indeed aligns
to 16 byte boundary here)
Honza
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C++ patch] Reduce vtable alignment
2014-05-19 8:52 ` Richard Biener
2014-05-19 9:10 ` Jakub Jelinek
@ 2014-05-23 17:30 ` Jan Hubicka
2014-05-23 21:52 ` Jason Merrill
1 sibling, 1 reply; 7+ messages in thread
From: Jan Hubicka @ 2014-05-23 17:30 UTC (permalink / raw)
To: Richard Biener; +Cc: Jan Hubicka, GCC Patches, Jason Merrill
Hi,
I would like to ping these two patches. If we conclude it is absolutely unsafe
to not align virtual tables to 16byte boundary (that is an x86_64 ABI
requirement for array datastructures but I would like to argue that vtables are
compiler controlled ones and do not need to follow ABI here), I can add a code
to while program visibility pass to bump up alignment of vtables that are
externally visible.
Vtables are always accessed via the vtbl pointer otherwise (that is almost
always misaligned because of the offset to RTTI pointer), so for vtables static
to given compilation unit, there is no way other compiler can derive the
alignment based on ABI promises. This would save the data segment size more progressively
at least for -flto.
Honza
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C++ patch] Reduce vtable alignment
2014-05-23 17:30 ` Jan Hubicka
@ 2014-05-23 21:52 ` Jason Merrill
0 siblings, 0 replies; 7+ messages in thread
From: Jason Merrill @ 2014-05-23 21:52 UTC (permalink / raw)
To: Jan Hubicka, Richard Biener; +Cc: GCC Patches
On 05/23/2014 01:30 PM, Jan Hubicka wrote:
> Vtables are always accessed via the vtbl pointer otherwise (that is almost
> always misaligned because of the offset to RTTI pointer), so for vtables static
> to given compilation unit, there is no way other compiler can derive the
> alignment based on ABI promises.
This makes sense to me. There's no reason why a compiler would rely on
the alignment of the vtable as a whole, since access is always to a
particular element.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-05-23 21:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-16 18:04 [C++ patch] Reduce vtable alignment Jan Hubicka
2014-05-16 19:12 ` Jan Hubicka
2014-05-19 8:52 ` Richard Biener
2014-05-19 9:10 ` Jakub Jelinek
2014-05-19 14:57 ` Jan Hubicka
2014-05-23 17:30 ` Jan Hubicka
2014-05-23 21:52 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).