[cfe-dev] [RFC] Use preferred alignment instead of ABI alignment for complete object when applicable

[Xiangling Liao via cfe-dev]

On some targets, preferred alignment is larger than ABI alignment in some
cases. For example, on AIX we have special power alignment rules which
would cause that. The full discussion details can be found here if
interested: http://lists.llvm.org/pipermail/cfe-dev/2020-April/065304.html .



Previously, to support those cases, we added a “PreferredAlignment” field
in the `RecordLayout` to store the AIX special alignment values in
“PreferredAlignment” as the community suggested. It makes sure both
`_alignof` and `alignof` return correct values for AIX objects. (The
related patch is here: https://reviews.llvm.org/D79719 .)



However, that patch alone is not enough. With AIX’s special alignment rule,
we need to use “PreferredAlignment” for a majority of the places where ABI
alignment are currently used in .



Take the cookie size for array new operator calculation for example, the
clang invokes `*getTypeAlignInChars` *to get ABI alignment value:


*CharUnits ItaniumCXXABI::getArrayCookieSizeImpl(QualType elementType) {*

*  // The array cookie is a size_t; pad that up to the element alignment.*

*  // The cookie is actually right-justified in that space.*

*  return std::max(CharUnits::fromQuantity(CGM.SizeSizeInBytes),*

*
CGM.getContext().getTypeAlignInChars(elementType));*

*}*

So far, we identify that there are 4 functions returning ABI alignment:

1) getTypeAlignInChars [invokes getTypeAlign]

2) getTypeAlign <=> getTypeInfo(T).Align
<https://clang.llvm.org/doxygen/structclang_1_1TypeInfo.html#af98dcefc364629eff868a1bb4010ebd8>

3) getTypeInfo [ invokes ‘getTypeInfoImpl’]

4) getTypeInfoInChars



Some possible ways we can think of are:

*1. Create a new function to be invoked instead*, e.g:

*/// getTypeProperAlignInChars - Return the ABI-specified alignment of a
type, in*

*/// characters. Except on AIX, return the preferred type alignment instead
when*

*/// under power alignment rules.*

*CharUnits ASTContext::getTypeProperAlignInChars(QualType T) const {*

*  if (Target->defaultsToAIXPowerAlignment())*

*    return toCharUnitsFromBits(getPreferredTypeAlign(T.getTypePtr()));*



*  return getTypeAlignInChars(T);*

*}*



Take the calculation of cookie size for array new operator for example, we
need:



*CharUnits ItaniumCXXABI::getArrayCookieSizeImpl(QualType elementType) {*

*  // The array cookie is a size_t; pad that up to the element alignment.*

*  // The cookie is actually right-justified in that space.*

*  return std::max(CharUnits::fromQuantity(CGM.SizeSizeInBytes),*

*-
CGM.getContext().getTypeAlignInChars(elementType));*

*+
CGM.getContext().getTypeProperAlignInChars(elementType));*

*}*



*Cons:*

The migration risk is high. We cannot guarantee in the future the
developers would know this is the interface they should use as always for
any common path code. So it seems this is not a viable method.



*2. Add a flag *

Adding a flag like the following can help us:



e.g.1 – getTypeAlignInChars

/// If `* NeedsABIAlignment *` is false, this function will smartly return
preferred alignment

/// or ABI alignment depends on if the target defaults to AIX power
alignment or not;

/// Set `* NeedsABIAlignment *` as true if an ABI alignment is needed no
matter what target is.

CharUnits ASTContext::getTypeAlignInChars(QualType T, *bool
NeedsABIAlignment = false*) const {
   if (NeedsABIAlignment)
     return toCharUnitsFromBits(getTypeAlign(T));
   if (Target->defaultsToAIXPowerAlignment())
     return toCharUnitsFromBits(getPreferredTypeAlign(T.getTypePtr()));



   return toCharUnitsFromBits(getTypeAlign(T));
}


Take cookie size for array new operator calculation for example again, we
don’t need to make any change anymore.




e.g.2 - getTypeInfoImpl

TypeInfo ASTContext::getTypeInfoImpl(const Type *T, *bool *
*NeedsABIAlignment** = false*) const {

...

  switch (T->getTypeClass()) {

...

    case Type::Record:

        const auto *RT = cast<RecordType>(TT);

        const RecordDecl *RD = RT->getDecl();

        const ASTRecordLayout &Layout = getASTRecordLayout(RD);

        Width = toBits(Layout.getSize());

*+     if (NeedsABIAlignment)*

*+       **Align = toBits(Layout.getAlignment());*

*+     else if (**Target->defaultsToAIXPowerAlignment()**)*

*+       Align = toBits(Layout.getPreferredAlignment());*

*+     else*

*          Align = toBits(Layout.getAlignment());*

        AlignIsRequired = RD->hasAttr<AlignedAttr>();

        break;

  }

...

}


*An usage example on the call site:*

<Clang/ lib/AST/RecordLayoutBuilder.cpp>

  auto setDeclInfo = [&](bool IsIncompleteArrayType) {

-   TypeInfo TI = Context.getTypeInfo(D->getType());

+  TypeInfo TI = Context.getTypeInfo(D->getType(), true/* NeedsABIAlignment
*/);

    FieldAlign = Context.toCharUnitsFromBits(TI.Align);

    // Flexible array members don't have any size, but they have to be

    // aligned appropriately for their element type.

    EffectiveFieldSize = FieldSize =

        IsIncompleteArrayType ? CharUnits::Zero()

                              : Context.toCharUnitsFromBits(TI.Width);

    AlignIsRequired = TI.AlignIsRequired;

  };



*Pros:*

1) By adding a flag like above, we can minimize the changes needed to make.
The scope of changes will be shortened to only a few spots where an ABI
alignment is necessary even when AIX special alignment is set.

2) Letting people consciously pass `true`  in if needed is another benefit.
Then targets like AIX don’t need to worry about things going wrong
accidentally in the future.





*cons:*

Those four functions I mentioned above won’t return ABI-specified alignment
values all the time anymore.





I would like to see how people think about our proposal. Any concerns? Or
any other ideas to suggest?



Please let me know if there are any questions about our current proposal
and design. Your feedback is appreciated.



Regards,



Xiangling Liao
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200812/52b86f51/attachment-0001.html>