[PATCH] D107394: [AIX] "aligned" attribute does not decrease alignment

[John McCall via Phabricator via cfe-commits]

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