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

James Y Knight via cfe-dev cfe-dev at lists.llvm.org
Mon Aug 17 13:06:44 PDT 2020


It looks to me as if at least examples 1, 2, and 4 are places that should
*not* be changed to use the "AIX preferred" alignment, because they are
inquiring about the guaranteed alignment of an existing pointer, which may
well not have been placed at the preferred alignment. I haven't dug into
the code beyond your snippet so I'm not sure about examples 3 and 5.

Are you saying you think all 5 of these examples should be using the AIX
preferred alignment? Or are you just listing locations that call
getTypeAlign and therefore need to be examined?


On Wed, Aug 12, 2020, 5:26 PM Xiangling Liao <xiangxdh at gmail.com> wrote:

> 1. "a real name is more readable"
>
> Hi Eli, could you elaborate a little more on what real name you are
> expecting?
>
>
>
> 2. "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."
>
>
>
> Based on the comment on `getTypeAlignInChars`, "*Return the ABI-specified
> alignment of a type, in characters. This method does not work on incomplete
> types*.", I believe `getTypeAlignInChars` and all other three functions I
> mentioned earlier(which have similar comments) are designed for getting ABI
> alignment. And these functions are used pervasively in Clang code.  So we
> need to fix all the places where an "AIX preferred" alignment is wanted
> when the target defaults to AIX power alignment. And the problem we are
> trying to deal with is how do we fix those alignment issues. So far we can
> think of:
>
>
>
> 1) Replace "getTypeAlign" with the following all over the places(which I
> think this chunk would make the code a bit ugly, and also has migration
> risk)
>
>
>
> *DefaultsToAIXPowerAlignment*
>
> *        ? getPreferredTypeAlign*
>
> *         : getTypeAlign*
>
> 2) Wrap above with a new function with migration risk as I stated before
>
> 3) Add a flag as I stated before
>
>
>
> Any other ideas are also welcome.
>
> 3. “please specify what the other cases are that need to change”
>
> The usage of those functions are way too many to be listed all
> here(probably at least 50 spots), so I just picked several residing on the
> common path for all targets.
>
>
>
> Eg.1:
>
> <lib/Sema/SemaChecking.cpp>
>
> static CharUnits getPresumedAlignmentOfPointer(const Expr *E, Sema &S) {
>
>   // See if we can compute the alignment of a VarDecl and an offset from
> it.
>
>   Optional<std::pair<CharUnits, CharUnits>> P =
>
>       getBaseAlignmentAndOffsetFromPtr(E, S.Context);
>
>
>
>   if (P)
>
>     return P->first.alignmentAtOffset(P->second);
>
>
>
>   // If that failed, return the type's alignment.
>
>   return S.Context.getTypeAlignInChars(E->getType()->getPointeeType());
>
> }
>
>
>
> Eg.2:
>
> <lib/Sema/SemaChecking.cpp>
>
> void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
>
>   E = E->IgnoreParens();
>
>   if (!T->isPointerType() && !T->isIntegerType())
>
>     return;
>
>   if (isa<UnaryOperator>(E) &&
>
>       cast<UnaryOperator>(E)->getOpcode() == UO_AddrOf) {
>
>     auto *Op = cast<UnaryOperator>(E)->getSubExpr()->IgnoreParens();
>
>     if (isa<MemberExpr>(Op)) {
>
>       auto MA = llvm::find(MisalignedMembers, MisalignedMember(Op));
>
>       if (MA != MisalignedMembers.end() &&
>
>           (T->isIntegerType() ||
>
>            (T->isPointerType() &&
> (T->getPointeeType()->isIncompleteType() ||
>
>                                    Context.getTypeAlignInChars(
>
>                                        T->getPointeeType()) <=
> MA->Alignment))))
>
>         MisalignedMembers.erase(MA);
>
>     }
>
>   }
>
> }
>
>
>
> Eg.3:
>
> < lib/CodeGen/CGOpenMPRuntime.cpp>
>
> Address CGOpenMPRuntime::getOrCreateDefaultLocation(unsigned Flags) {
>
>   CharUnits Align = CGM.getContext().getTypeAlignInChars(IdentQTy);
>
>   unsigned Reserved2Flags = getDefaultLocationReserved2Flags();
>
>   ...
>
> }
>
>
>
> Eg.4:
>
> < lib/CodeGen/CGAtomic.cpp>
>
> static void
>
> AddDirectArgument(CodeGenFunction &CGF, CallArgList &Args,
>
>                   bool UseOptimizedLibcall, llvm::Value *Val, QualType
> ValTy,
>
>                   SourceLocation Loc, CharUnits SizeInChars) {
>
>   if (UseOptimizedLibcall) {
>
>     // Load value and pass it to the function directly.
>
>     CharUnits Align = CGF.getContext().getTypeAlignInChars(ValTy);
>
> ...
>
> }
>
>
>
> Eg.5:
>
> < lib/CodeGen/TargetInfo.cpp>
>
> ABIArgInfo ABIInfo::getNaturalAlignIndirect(QualType Ty, bool ByVal,
>
>                                             bool Realign,
>
>                                             llvm::Type *Padding) const {
>
>   return ABIArgInfo::getIndirect(getContext().getTypeAlignInChars(Ty),
> ByVal,
>
>                                  Realign, Padding);
>
> }
>
>
> Thank you,
>
> Xiangling
>
>
> On Wed, Aug 12, 2020 at 3:08 PM James Y Knight <jyknight at google.com>
> wrote:
>
>> 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/20200817/01d5a46e/attachment-0001.html>


More information about the cfe-dev mailing list