[cfe-dev] [RFC] Use preferred alignment instead of ABI alignment for complete object when applicable
David Rector via cfe-dev
cfe-dev at lists.llvm.org
Mon Aug 17 21:54:52 PDT 2020
> On Aug 17, 2020, at 4:06 PM, James Y Knight via cfe-dev <cfe-dev at lists.llvm.org> wrote:
>
> 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?
I would have to think that, whatever the documentation for e.g. getTypeAlignInChars(T), most users expect it to return T’s actual alignment for their target, whether it was determined from the ABI, or the AIX alignment scheme, or whatever.
In fact, maybe we can go further: I would think that any valid use of type alignment which explicitly ignores AIX or other target-specific alignment schemes are probably just implementation details used only in the calculation of some composed type’s alignment under that scheme. In other words I would assume every function call in which AIX preferred alignment needed to be ignored is in the some layout-building implementation — is that right?
E.g. the lib/AST/RecordLayoutBuilder.cpp example you cite below fits this description — in LayoutField(FieldDecl *D, …) you propose to change the call to fetch the ABI/non-AIX type info via
getTypeInfo(D->getType(), true/* NeedsABIAlignment */);
but only so that you can later tweak that info whenever DefaultsToAIXPowerAlignment==true.
If all the changes you would need to make are indeed similar to this, these expanded usages are really only implementation details needed to build type layouts, and thus the goal should simply be to sufficiently hide these details from common programmers who don’t need to build type layouts, only access them.
I think your original option #2 ("add a flag"), i.e. adding new defaulted parameters to a few functions, seems to be a reasonable way to hide these new details, so long as the new parameters are well named and documented. It also seems sufficiently general to be useful for implementing other special default alignment schemes in the future.
Dave
>
>
> On Wed, Aug 12, 2020, 5:26 PM Xiangling Liao <xiangxdh at gmail.com <mailto: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 <mailto: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 <mailto: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 <mailto:xiangxdh at gmail.com>>
> Sent: Wednesday, August 12, 2020 10:56 AM
> To: cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> Cc: hubert.reinterpretcast at gmail.com <mailto:hubert.reinterpretcast at gmail.com>; jasonliu.development at gmail.com <mailto:jasonliu.development at gmail.com>; Eli Friedman <efriedma at quicinc.com <mailto:efriedma at quicinc.com>>; James Y Knight <jyknight at google.com <mailto:jyknight at google.com>>; Reid Kleckner <rnk at google.com <mailto: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 <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 <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
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200818/28d2531b/attachment-0001.html>
More information about the cfe-dev
mailing list