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

[James Y Knight via cfe-dev]

Agree with Eli -- please specify what the other cases are that need to
change, besides the alignment used for "new".

On Wed, Aug 12, 2020 at 2:46 PM Eli Friedman <efriedma at quicinc.com> wrote:

> I don’t think using a boolean argument is appropriate here; nobody is
> going to pass in anything other than constant true/false, so a real name is
> more readable.
>
>
>
> I guess the question here is what the “default” getTypeAlign means; either
> it’s the ABI alignment, and we fix all the places that want the “AIX
> preferred” alignment, or it’s the “AIX preferred” alignment, and we fix all
> the places that want the ABI alignment.  I think it’s essentially a numbers
> game: which one is actually more commonly used?  Please give more examples.
>
>
>
> -Eli
>
>
>
> *From:* Xiangling Liao <xiangxdh at gmail.com>
> *Sent:* Wednesday, August 12, 2020 10:56 AM
> *To:* cfe-dev at lists.llvm.org
> *Cc:* hubert.reinterpretcast at gmail.com; jasonliu.development at gmail.com;
> Eli Friedman <efriedma at quicinc.com>; James Y Knight <jyknight at google.com>;
> Reid Kleckner <rnk at google.com>
> *Subject:* [EXT] [RFC] Use preferred alignment instead of ABI alignment
> for complete object when applicable
>
>
>
> 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/3d82815a/attachment-0001.html>