Xiangling_L marked 27 inline comments as done. Xiangling_L added inline comments.
================ Comment at: clang/lib/AST/ASTContext.cpp:2424 + (T->isSpecificBuiltinType(BuiltinType::LongDouble) && + Target->supportsAIXPowerAlignment())) // Don't increase the alignment if an alignment attribute was specified on a ---------------- hubert.reinterpretcast wrote: > hubert.reinterpretcast wrote: > > Xiangling_L wrote: > > > hubert.reinterpretcast wrote: > > > > Does `supportsAIXPowerAlignment` express the condition we want to check > > > > here? That might be true for an implementation operating with `mac68k` > > > > alignment rules. > > > Yeah, `supportsAIXPowerAlignment` cannot separate the preferred alignment > > > of double, long double between `power/natural` and `mac68k` alignment > > > rules. But I noticed that currently, AIX target on wyvern or XL don't > > > support `mac68k` , so maybe we should leave further changes to the patch > > > which is gonna implement `mac68k` alignment rules? The possible solution > > > I am thinking is we can add checking if the decl has `AlignMac68kAttr` > > > into query to separate things out. > > > > > > Another thing is that once we start supporting mac68k alignment rule(if > > > we will), should we also change the ABI align values as well? (e.g. for > > > double, it should be 2 instead) > > If the "base state" is AIX `power` alignment for a platform, I suggest that > > the name be `defaultsToAIXPowerAlignment`. > This last question about the ABI align values is relevant to considerations > for `natural` alignment support as well. More generally, the question is > whether the "minimum alignment" of the type in a context subject to > alternative alignment rules is altered to match said alignment rule. This is > observable via the diagnostic associated with C++11 alignment specifiers. > > The existing behaviour of `mac68k` alignment suggests that the "minimum > alignment" is context-free. > > ``` > #pragma options align=mac68k > struct Q { > double x alignas(2); // expected-error {{less than minimum alignment}} > }; > #pragma options align=reset > ``` > > Compiler Explorer link: https://godbolt.org/z/9NM5_- Thank you for your explanation. ================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1885 + Context.getBaseElementType(CTy->getElementType()) + ->getAs<BuiltinType>()) + if (BTy->getKind() == BuiltinType::Double || ---------------- hubert.reinterpretcast wrote: > I believe `castAs` should be expected to succeed here. `castAs` is not declared in current context, do we really want to use it by introducing one more header? ================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1898 + } else if (const RecordType *RT = D->getType() + ->getBaseElementTypeUnsafe() + ->getAs<RecordType>()) { ---------------- hubert.reinterpretcast wrote: > Is there a reason to use `getBaseElementTypeUnsafe` for this case and > `Context.getBaseElementType` for the other ones? Also, it would make sense to > factor out the array-type considerations once at the top of the if-else chain > instead of doing so in each alternative. Sorry, I didn't pay attention to the different versions of `getBaseElementType` functions here and I believe this part of code came from our old compiler's changesets. My understanding would be since type qualifiers are not very meaningful in our case and `getBaseElementTypeUnsafe()` is more efficient than `getBaseElementType()`, we can use `getBaseElementTypeUnsafe()` all over the place instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79719/new/ https://reviews.llvm.org/D79719 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits