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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
