Message ID | 20140516191242.GA10833@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
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
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
> > 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
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
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
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