stevewan marked 2 inline comments as done.
stevewan added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1975
+ bool AlignAttrCanDecreaseAlignment =
+ AlignIsRequired && (Ty->getAs<TypedefType>() != nullptr || FieldPacked);
+
----------------
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.
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
