[AIX] "aligned" attribute does not decrease alignment

[stevewan]

Summary

The "aligned" attribute can only increase the alignment of a struct, or struct member, unless it's used together with the "packed" attribute, or used as a part of a typedef, in which case, the "aligned" attribute can both increase and decrease alignment.

That said, we expect:

1. "aligned" attribute alone: does not interfere with the alignment upgrade instrumented by the AIX "power" alignment rule,
2. "aligned" attribute + typedef: overrides any computed alignment,
3. "aligned" attribute + "packed" attribute: overrides any computed alignment.

The old implementation achieved 2 and 3, but didn't get 1 right, in that any field marked attribute "aligned" would not go through the alignment upgrade.

[stevewan]

Attr 'packed' is already taken into account in ABIAlign/PreferredAlign

[stevewan]

Fix incomplete diff, please discard last update.

[sfertile]

clang/test/Layout/aix-alignof-align-and-pack-attr.cpp

11

Minor nit: I think the other test can live in this file, highlighting the difference between the 2 cases is nice. Also I think we should add a typedef test in here as well.

[stevewan]

Merge tests into one file

[stevewan]

clang/test/Layout/aix-alignof-align-and-pack-attr.cpp

11

Merged the tests as suggested. We have the typedef coverage in aix-power-alignment-typedef.cpp and aix-power-alignment-typedef2.cpp, not sure if want to merge those as well.

[sfertile]

Thanks Steven, LGTM.

clang/test/Layout/aix-alignof-align-and-pack-attr.cpp

11

No need to merge those as well.

[stevewan]

Fix the same problem in field layout.

[stevewan]

Add test cases

[stevewan]

The query portion has been split out as https://reviews.llvm.org/D107598, which needs to be cleaned out from this patch. The record layout part also needs update as we uncover more problems with the current implementation.

[stevewan]

Rebase to latest main

[stevewan]

Fields marked attribute "aligned" and "packed" don't need to go through alignment upgrade

[rjmccall]

clang/lib/AST/RecordLayoutBuilder.cpp

1975 ↗

(On Diff #367312)

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.

[stevewan]

Address comments about the check

[stevewan]

clang/lib/AST/RecordLayoutBuilder.cpp

1975 ↗

(On Diff #367312)

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.

[stevewan]

De-Morgan-ize if condition for readability

[stevewan]

Fix typo

[rjmccall]

clang/lib/AST/RecordLayoutBuilder.cpp

1975 ↗

(On Diff #367312)

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.

[stevewan]

Add recursive check for typedef

[stevewan]

clang/lib/AST/RecordLayoutBuilder.cpp

1975 ↗

(On Diff #367312)

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.

[stevewan]

Avoid unannotated fall-through between cases

[rjmccall]

clang/lib/AST/RecordLayoutBuilder.cpp

1984 ↗

(On Diff #368716)

Hah. The knot-tying here is really cute, but I think either just make it a static function somewhere in the file, or make it iterative instead of recursive.

...actually, I think it would be better to just make the computation in getTypeInfo preserve more information. Make TypeInfo carry a three-valued enum instead of an AlignIsRequired flag:

enum class AlignRequirement {
  /// The alignment was not explicit in code.
  None,

  /// The alignment comes from an alignment attribute on a typedef.
  RequiredByTypedef,

  /// The alignment comes from an alignment attribute on a record type.
  RequiredByRecord,
};

You can add a method on TypeInfo to ask if the alignment is required for any reason and make all the existing clients use that instead.

[stevewan]

Make TypeInfo carry more information about "aligned" attribute

[stevewan]

clang/lib/AST/RecordLayoutBuilder.cpp

1984 ↗

(On Diff #368716)

Good idea. I had thought about a similar approach but was afraid that the change may get too pervasive. I think it turned out fine. I'm now debating whether I should split this whole bool-to-enum change into a separate patch, any preference?

[rjmccall]

Thanks, style suggestion aside, this looks great. Would you mind doing the TypeInfo change in a separate patch? The functional change for AIX can be a really small improvement on top of that.

clang/include/clang/AST/ASTContext.h

167 ↗

(On Diff #369157)

We've been calling things like this FooKind for a while. FooType is pretty problematic in a compiler where we have a lot of type representations. I think there are a few grandfathered exceptions, but still, let's call it AlignRequirementKind.

[stevewan]

Cleanup the condition check

[stevewan]

Fix typo

[stevewan]

Fix missing "&&"

[rjmccall]

Please add me to the review for the other patch and I'll approve it pretty quickly, and then we can get back to this one.

[stevewan]

In D107394#2970326, @rjmccall wrote:

Please add me to the review for the other patch and I'll approve it pretty quickly, and then we can get back to this one.

Sorry for the delay. Posted https://reviews.llvm.org/D108858, once that's in I'll rebase.

[stevewan]

Adapt to new TypeInfo design

[rjmccall]

Thanks, this looks a lot cleaner. Minor requests only.

[rjmccall]

clang/lib/AST/RecordLayoutBuilder.cpp

1975 ↗

(On Diff #367312)

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.

[stevewan]

Rename lambda and add comments

[rjmccall]

Thanks, LGTM!

clang/lib/AST/ASTContext.cpp

unsigned ASTContext::getPreferredTypeAlign(const Type *T) const {
   // The preferred alignment of member pointers is that of a pointer.
   if (T->isMemberPointerType())
     return getPreferredTypeAlign(getPointerDiffType().getTypePtr());
 
   if (!Target->allowsLargerPreferedTypeAlignment())
     return ABIAlign;
 
   if (const auto *RT = T->getAs<RecordType>()) {
-    if (TI.AlignIsRequired || RT->getDecl()->isInvalidDecl())
+    const RecordDecl *RD = RT->getDecl();
+
+    // When used as part of a typedef, or together with a 'packed' attribute,
+    // the 'aligned' attribute can be used to decrease alignment.
+    if ((TI.AlignIsRequired && T->getAs<TypedefType>() != nullptr) ||
+        RD->isInvalidDecl())
       return ABIAlign;
 
     unsigned PreferredAlign = static_cast<unsigned>(
-        toBits(getASTRecordLayout(RT->getDecl()).PreferredAlignment));
+        toBits(getASTRecordLayout(RD).PreferredAlignment));
     assert(PreferredAlign >= ABIAlign &&
            "PreferredAlign should be at least as large as ABIAlign.");
     return PreferredAlign;
   }
 
   // Double (and, for targets supporting AIX `power` alignment, long double) and
   // long long should be naturally aligned (despite requiring less alignment) if
   // possible.

clang/test/Layout/aix-alignof-align-and-pack-attr.cpp

+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -S -emit-llvm -x c++ < %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -S -emit-llvm -x c++ < %s | \
+// RUN:   FileCheck %s
+
+struct __attribute__((__aligned__(2), packed)) C {
+  double x;
+} c;
+
+// CHECK: @c = global %struct.C zeroinitializer, align 2

clang/test/Layout/aix-alignof-align-attr.cpp

+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -S -emit-llvm -x c++ < %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -S -emit-llvm -x c++ < %s | \
+// RUN:   FileCheck %s
+
+struct __attribute__((__aligned__(2))) C {
+  double x;
+} c;
+
+// CHECK: @c = global %struct.C zeroinitializer, align 8