This is an archive of the discontinued LLVM Phabricator instance.

[RTTI] Align rtti types to prevent over-alignment
ClosedPublic

Authored by dmgreen on Aug 29 2018, 5:30 AM.

Details

Summary

Previously the alignment on the newly created rtti/typeinfo data was largely
not set, meaning that DataLayout::getPreferredAlignment was free to overalign
it to 16 bytes. This causes unnecessary code bloat.

Diff Detail

Event Timeline

dmgreen created this revision.Aug 29 2018, 5:30 AM

Could we make CreateOrReplaceCXXRuntimeVariable take the alignment as an argument, so we can be sure we're consistently setting the alignment for these variables? This seems easy to mess up if we're scattering calls all over the place.

Sort of orthogonal, but CreateOrReplaceCXXRuntimeVariable should probably also call setUnnamedAddr, instead of making each caller call it.

dmgreen updated this revision to Diff 163727.Sep 3 2018, 9:07 AM
dmgreen retitled this revision from [RTTI] Align rtti type string to prevent over-alignment to [RTTI] Align rtti types to prevent over-alignment.
dmgreen edited the summary of this revision. (Show Details)

I've become less sure about what the various alignments should be here. There seem to be multiple ways to calculate such things, I'm not sure which are best. For example, for want of a better option I've used getPointerAlign in ItaniumRTTIBuilder::BuildTypeInfo.

getClassPointerAlignment is not really relevant here; that's the alignment of a pointer to an object with the type of the class.

For the LLVM IR structs which don't have a corresponding clang type, you can probably just use DataLayout::getABITypeAlignment(). I think that's just the alignment of a pointer in practice, but the intent is more clear.

lib/CodeGen/MicrosoftCXXABI.cpp
2027

This is an array of int.

dmgreen updated this revision to Diff 164693.Sep 10 2018, 10:13 AM

Now using getABITypeAlignment, plus added a simple test

efriedma accepted this revision.Sep 11 2018, 1:01 PM

LGTM with one change.

lib/CodeGen/ItaniumCXXABI.cpp
2730

Please use getTypeAlignInChars(getContext().CharTy), or something like that, so it's clear where the "1" is coming from.

This revision is now accepted and ready to land.Sep 11 2018, 1:01 PM

That sounds like a good idea. Thanks for the help on this one.

This revision was automatically updated to reflect the committed changes.