[PATCH] D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 23 11:41:57 PDT 2018


rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm, I was hoping @rsmith would have a better suggestion, but we don't need to wait on it.



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:90
+        reinterpret_cast<const char *>(this) +
+        offsetof(SmallVectorAlignmentAndSize<T>, FirstEl)));
+  }
----------------
dexonsmith wrote:
> dexonsmith wrote:
> > rnk wrote:
> > > This assumes that base classes are laid out more or less the same as fields, but we were already assuming that there wouldn't be padding between FirstEl and InlineElts, so this seems like an improvement.
> > What about this?
> > ```
> > struct SmallVectorAlignmentAndSizeBase {
> >   AlignedCharArrayUnion<SmallVectorBase> Base;
> > };
> > template <class T>
> > struct SmallVectorAlignmentAndSize : SmallVectorAlignmentAndSizeBase {
> >   AlignedCharArrayUnion<T> FirstEl;
> > };
> > 
> > // still doing this later:
> >   void *getFirstEl() const {
> >     return const_cast<void *>(reinterpret_cast<const void *>(
> >         reinterpret_cast<const char *>(this) +
> >         offsetof(SmallVectorAlignmentAndSize<T>, FirstEl)));
> >   }
> > ```
> > we were already assuming that there wouldn't be padding between FirstEl and InlineElts
> 
> BTW, I don't think we had to assuming that there wouldn't be padding.  As long as there's enough otherwise-unused storage after `FirstEl`, we won't overflow the allocation.
I think there are interesting differences between multiple inheritance and single inheritance, so I don't think this is an improvement over the simpler `offsetof` calculation that you have now.


https://reviews.llvm.org/D49163





More information about the llvm-commits mailing list