[C++ patch] Reduce vtable alignment

[C++ patch] Reduce vtable alignment

[Jan Hubicka]

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;

Re: [C++ patch] Reduce vtable alignment

[Jan Hubicka]

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

Re: [C++ patch] Reduce vtable alignment

[Richard Biener]

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

Re: [C++ patch] Reduce vtable alignment

[Jakub Jelinek]

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

Re: [C++ patch] Reduce vtable alignment

[Jan Hubicka]

> > 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

Re: [C++ patch] Reduce vtable alignment

[Jan Hubicka]

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

Re: [C++ patch] Reduce vtable alignment

[Jason Merrill]

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