rjmccall added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1975
+ bool AlignAttrCanDecreaseAlignment =
+ AlignIsRequired && (Ty->getAs<TypedefType>() != nullptr || FieldPacked);
+
----------------
stevewan wrote:
> rjmccall wrote:
> > stevewan wrote:
> > > rjmccall wrote:
> > > > Okay, so first off, the comment and variable names here make this sound
> > > > very general when in fact this computation is only relevant to the AIX
> > > > alignment upgrade logic. Also, please do not introduce new variables
> > > > with broad scope named things like `Ty` that are actually referring to
> > > > a very specific type and not, say, the unadulterated type of the field.
> > > >
> > > > It seems to me that the right way to think about this is that, while
> > > > the AIX power alignment upgrade considers whether field type alignment
> > > > is "required", it uses a different condition than the standard rule
> > > > when making that decision. It's important to understand what that rule
> > > > is exactly, and we can't see that from the current test cases. In
> > > > particular, attributes on fields that define structs inline actually
> > > > end up applying to both the field and the struct, which confounds
> > > > unrelated issues. Consider:
> > > >
> > > > ```
> > > > struct __attribute__((packed, aligned(2))) SS {
> > > > double d;
> > > > };
> > > >
> > > > struct S {
> > > > // Neither the field nor the struct are packed, but the field type is.
> > > > // Do we apply the alignment upgrade to S or not?
> > > > struct SS s;
> > > > };
> > > > ```
> > > >
> > > > Regardless, I don't think this check for a typedef works; it's
> > > > bypassing quite a bit of recursive logic for computing whether type
> > > > alignment is required. For example, you could have an explicitly
> > > > aligned typedef of an array type, and you'll lose that typedef here.
> > > Thanks for the review.
> > >
> > > > the comment and variable names here make this sound very general when
> > > > in fact this computation is only relevant to the AIX alignment upgrade
> > > > logic.
> > > Moved the variable declarations back to the AIX alignment upgrade logic.
> > > > It's important to understand what that rule is exactly, and we can't
> > > > see that from the current test cases.
> > > Updated the test cases accordingly.
> > > > For example, you could have an explicitly aligned typedef of an array
> > > > type, and you'll lose that typedef here.
> > > Updated the check to examine the immediate type rather than the base
> > > element type, this should take care of the array-type considerations.
> > >
> > No, I'm not sure that's quite correct, either, because you might have an
> > array of an explicitly aligned typedef. You might need to do a recursive
> > examination of the type the same way that getTypeInfo would, except I guess
> > not recursing into records.
> >
> > I assume you verified that the behavior in your new test cases matches the
> > behavior of some existing AIX compiler? For our general edification, what
> > compiler(s) are you testing against?
> >
> > I've found that extracting very complex conditions into a function (perhaps
> > a lambda if it's inconvenient to have it separately-declared) can be useful.
> Added a lambda function that mimics getTypeInfo to do the recursive check. If
> this looks about right in direction, I'm also going to memorize known types'
> typedef-ness in a map for faster lookup.
>
> Yes, the behaviour was verified, and matches the behaviour of XL 16.1 on AIX.
Somehow my requests didn't make it through. Please name this lambda something
AIX-specific, and please clarify in a comment that we can ignore enum alignment
sources because the alignment upgrade only applies to floating-point types.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107394/new/
https://reviews.llvm.org/D107394
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
