Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

[uweigand]

Summary

The SystemZ ABI has special cases to handle structs containing just a single floating-point member. In determining this property, there are corner cases around empty fields. So for example, if a struct contains an "empty member" in addition to a member of floating-point type, the struct is still considered to contain just a single floating-point member.

In (prior versions of) C++, however, members of class type would never count as "empty" given the C++ rules that even an empty class should be considered as having a size of at least 1. But now with C++20, data members can be marked with the [[no_unique_address]] attribute, in which case that rule no longer applies. The Itanium ABI document was updated to address this new situation (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#definitions):

empty class

A class with no non-static data members other than empty data members, no unnamed bit-fields other than zero-width bit-fields, no virtual functions, no virtual base classes, and no non-empty non-virtual proper base classes.

empty data member

A potentially-overlapping non-static data member of empty class type.

GCC 10 has been updated across platforms to respect this new case.

This patch implements the new case in the ABI code for SystemZ. Note that I'm changing common subroutines (isEmptyField / isEmptyRecord) that are used for other ABIs as well. To prevent this patch from having any unintended effect on other platforms, I've guarded the new behavior with an extra flag that is currently only set on SystemZ.

Now I would expect that most other platforms who use isEmptyField / isEmptyRecord / isSingleElementStruct / isHomogeneousAggregate will also have to respect that new behavior in order to stay compatible with the respective system ABIs (and GCC), but I'd prefer to leave this up to the maintainers of those platforms ...

[uweigand]

Ping? I'd really appreciate feedback about this ABI issue, in particular from other affected target maintainers ...

[uweigand]

Another ping ...

See also http://lists.llvm.org/pipermail/cfe-dev/2020-July/066162.html

[hubert.reinterpretcast]

clang/lib/CodeGen/TargetInfo.cpp

535

Minor nit:
The nested if could be merged with the outer one:

if (isa<CXXRecordDecl>(RT->getDecl()) &&
    !(AllowNoUniqueAddr && FD->hasAttr<NoUniqueAddressAttr>()))
  return false;

[uweigand]

Rebased against mainline.

[efriedma]

I'm tempted to say this is a bugfix for the implementation of no_unique_address, and just fix it globally for all ABIs. We're never going to get anything done here if we require a separate patch for each ABI variant clang supports.

clang/lib/CodeGen/TargetInfo.cpp

527

Does this do the right thing with a field that's an array of empty classes?

7249–7253

Only loosely relevant to this patch, but checking getLangOpts().CPlusPlus here seems weird; doesn't that break calling functions defined in C from C++ code?

[hubert.reinterpretcast]

clang/lib/CodeGen/TargetInfo.cpp

524

This check is being done after removal of the array types by AllowArrays, so this code is also conferring the property of being empty to arrays. It seems GCC erroneously does the same for base class fields (but not for direct members).

struct Empty {};

struct A {
  Empty emp [[no_unique_address]][3];
};

struct B : A {
  float f;
};

struct C {
  Empty emp [[no_unique_address]][3];
  float f;
};

extern char szb[sizeof(B)];
extern char szb[sizeof(float)]; // GCC likes this
extern char szc[sizeof(C)];
extern char szc[sizeof(float)]; // GCC does not like this

Compiler Explorer link: https://godbolt.org/z/NFuca9

7234

The Itanium ABI defines "empty data member" as:

A potentially-overlapping non-static data member of empty class type.

That definition does not include non-static data members of array type whose base element type is an empty class type.

7253

Should this be controlled by an -fclang-abi-compat option?

[qiucf]

Thanks for this patch!

If I understand correctly, only isEmptyRecord/isEmptyField and places checking any field is zero bit-width may need change for this? Since isSingleElementStruct and isHomogeneousAggregate just use them to skip empty fields in aggregate. I didn't see direct checking for empty fields on PowerPC. So all change needed on PPC seems to be generic. By enabling AllowNoUniqueAddr in these methods, case in https://lists.llvm.org/pipermail/llvm-dev/2020-July/143141.html can be correctly recognized as homogeneous aggregate.

[uweigand]

Handle array of empty records correctly.

[uweigand]

clang/lib/CodeGen/TargetInfo.cpp

524

This version should fix clang; I agree that GCC still gets this wrong.

527

You're right. As Hubert notes, arrays of empty classes do not count as empty. This version should fix the problem.

7249–7253

I agree that this difference between C and C++ is weird, but it does match the behavior of GCC. (Which is itself weird, but a long-standing accident that we probably cannot fix without breaking existing code at this point.)

Now, you bring up an interesting point: When C++ code calls a function defined in C code, the C++ part would have to refer to an extern "C" declaration. The correct thing to do would probably be to handle those according to the C ABI rules, not the C++ rules, in this case where the two differ. But currently GCC doesn't do that either. (But since that would be broken anyway, I think we can fix that.) In any case, I agree that this is really a separate problem, distinct from this patch.

[uweigand]

In D81583#2137277, @efriedma wrote:

I'm tempted to say this is a bugfix for the implementation of no_unique_address, and just fix it globally for all ABIs. We're never going to get anything done here if we require a separate patch for each ABI variant clang supports.

Well, I can certainly do that. Would you prefer me to completely remove the AllowNoUniqueAddress parameter, or keep it but default to true (so ABIs can still override if necessary)?

[uweigand]

In D81583#2138127, @qiucf wrote:

Thanks for this patch!

If I understand correctly, only isEmptyRecord/isEmptyField and places checking any field is zero bit-width may need change for this? Since isSingleElementStruct and isHomogeneousAggregate just use them to skip empty fields in aggregate. I didn't see direct checking for empty fields on PowerPC. So all change needed on PPC seems to be generic. By enabling AllowNoUniqueAddr in these methods, case in https://lists.llvm.org/pipermail/llvm-dev/2020-July/143141.html can be correctly recognized as homogeneous aggregate.

I agree that probably the only required change is to set the AllowNoUniqueAddr parameter to true in those methods. Given the discussion with @efriedma above, that may actually happen automatically if we remove the parameter (or default it to true).

In any case, I guess it would still be good to also have test cases for this aspect of the ABI on Power ...

[qiucf]

In any case, I guess it would still be good to also have test cases for this aspect of the ABI on Power ...

Sure, I'd like to do pre-commit for PPC tests. (or after this landed and enable/remove AllowNoUniqueAddr)

[rjmccall]

I agree with Eli that this should be considered a bugfix in the implementation of a recent language change and should just be rolled out consistently for all targets.

If this were a five-year-old feature, the ABI considerations would be different, but it's not.

[efriedma]

In D81583#2139002, @uweigand wrote:

In D81583#2137277, @efriedma wrote:

I'm tempted to say this is a bugfix for the implementation of no_unique_address, and just fix it globally for all ABIs. We're never going to get anything done here if we require a separate patch for each ABI variant clang supports.

Well, I can certainly do that. Would you prefer me to completely remove the AllowNoUniqueAddress parameter, or keep it but default to true (so ABIs can still override if necessary)?

Just drop it; I don't expect any ABI would want to override it.

clang/lib/CodeGen/TargetInfo.cpp

7249–7253

Okay.

Please move the check for NoUniqueAddressAttr out of the CPlusPlus check; I don't think it's currently possible to use from C, but better to be on the safe side.

[uweigand]

Drop AllowNoUniqueAddress parameter; address review comment.

[asb]

This LGTM from a RISC-V perspective. I'll likely follow up with a RISC-V test case similar to the SystemZ one post-commit, but given this is really fixing a cross-platform ABI issue this seems non-urgent. Thanks for spotting and addressing this issue.

[efriedma]

LGTM

[uweigand]

Committed as 4c5a93bd58bad70e91ac525b0e020bd5119a321a.

clang/lib/CodeGen/TargetInfo.cpp

static bool isEmptyField(ASTContext &Context, const FieldDecl *FD,
                          bool AllowArrays) {
   if (FD->isUnnamedBitfield())
     return true;
 
   QualType FT = FD->getType();
 
   // Constant arrays of empty records count as empty, strip them off.
   // Constant arrays of zero length always count as empty.
+  bool WasArray = false;
   if (AllowArrays)
     while (const ConstantArrayType *AT = Context.getAsConstantArrayType(FT)) {
       if (AT->getSize() == 0)
         return true;
       FT = AT->getElementType();
+      // The [[no_unique_address]] special case below does not apply to
+      // arrays of C++ empty records, so we need to remember this fact.
+      WasArray = true;
     }
 
   const RecordType *RT = FT->getAs<RecordType>();
   if (!RT)
     return false;
 
   // C++ record fields are never empty, at least in the Itanium ABI.
   //
   // FIXME: We should use a predicate for whether this behavior is true in the
   // current ABI.
-  if (isa<CXXRecordDecl>(RT->getDecl()))
+  //
+  // The exception to the above rule are fields marked with the
+  // [[no_unique_address]] attribute (since C++20).  Those do count as empty
+  // according to the Itanium ABI.  The exception applies only to records,
+  // not arrays of records, so we must also check whether we stripped off an
+  // array type above.
+  if (isa<CXXRecordDecl>(RT->getDecl()) &&
+      (WasArray || !FD->hasAttr<NoUniqueAddressAttr>()))
     return false;
 
   return isEmptyRecord(Context, FT, AllowArrays);
 }
 
 /// isEmptyRecord - Return true iff a structure contains only empty
 /// fields. Note that a structure with a flexible array member is not
 /// considered empty.
 static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays) {
   const RecordType *RT = T->getAs<RecordType>();
   if (!RT)
     return false;
   const RecordDecl *RD = RT->getDecl();
   if (RD->hasFlexibleArrayMember())
     return false;
if (RT && RT->isStructureOrClassType()) {
     QualType Found;
 
     // If this is a C++ record, check the bases first.
     if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD))
       for (const auto &I : CXXRD->bases()) {
         QualType Base = I.getType();
 
         // Empty bases don't affect things either way.
         if (isEmptyRecord(getContext(), Base, true))
           continue;
 
         if (!Found.isNull())
           return Ty;
         Found = GetSingleElementType(Base);
       }
 
     // Check the fields.
     for (const auto *FD : RD->fields()) {
       // For compatibility with GCC, ignore empty bitfields in C++ mode.
       // Unlike isSingleElementStruct(), empty structure and array fields
       // do count.  So do anonymous bitfields that aren't zero-sized.
       if (getContext().getLangOpts().CPlusPlus &&
           FD->isZeroLengthBitField(getContext()))
         continue;
+      // Like isSingleElementStruct(), ignore C++20 empty data members.
+      if (FD->hasAttr<NoUniqueAddressAttr>() &&
+          isEmptyRecord(getContext(), FD->getType(), true))
+        continue;
 
       // Unlike isSingleElementStruct(), arrays do not count.
       // Nested structures still do though.
       if (!Found.isNull())
         return Ty;
       Found = GetSingleElementType(FD->getType());
     }
 

clang/test/CodeGen/systemz-abi.cpp

 // For compatibility with GCC, this structure is passed in an FPR in C++,
 // but passed in a GPR in C (checked in systemz-abi.c).
 
 struct agg_float_cpp { float a; int : 0; };
 struct agg_float_cpp pass_agg_float_cpp(struct agg_float_cpp arg) { return arg; }
 // CHECK-LABEL: define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, float %{{.*}})
 // SOFT-FLOAT-LABEL:  define void @_Z18pass_agg_float_cpp13agg_float_cpp(%struct.agg_float_cpp* noalias sret align 4 %{{.*}}, i32 %{{.*}})
 
+
+// A field member of empty class type in C++ makes the record nonhomogeneous,
+// unless it is marked as [[no_unique_address]].  This does not apply to arrays.
+struct empty { };
+struct agg_nofloat_empty { float a; empty dummy; };
+struct agg_nofloat_empty pass_agg_nofloat_empty(struct agg_nofloat_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z22pass_agg_nofloat_empty17agg_nofloat_empty(%struct.agg_nofloat_empty* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct agg_float_empty { float a; [[no_unique_address]] empty dummy; };
+struct agg_float_empty pass_agg_float_empty(struct agg_float_empty arg) { return arg; }
+// CHECK-LABEL: define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z20pass_agg_float_empty15agg_float_empty(%struct.agg_float_empty* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+struct agg_nofloat_emptyarray { float a; [[no_unique_address]] empty dummy[3]; };
+struct agg_nofloat_emptyarray pass_agg_nofloat_emptyarray(struct agg_nofloat_emptyarray arg) { return arg; }
+// CHECK-LABEL: define void @_Z27pass_agg_nofloat_emptyarray22agg_nofloat_emptyarray(%struct.agg_nofloat_emptyarray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z27pass_agg_nofloat_emptyarray22agg_nofloat_emptyarray(%struct.agg_nofloat_emptyarray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+
+// And likewise for members of base classes.
+struct noemptybase { empty dummy; };
+struct agg_nofloat_emptybase : noemptybase { float a; };
+struct agg_nofloat_emptybase pass_agg_nofloat_emptybase(struct agg_nofloat_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z26pass_agg_nofloat_emptybase21agg_nofloat_emptybase(%struct.agg_nofloat_emptybase* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+struct emptybase { [[no_unique_address]] empty dummy; };
+struct agg_float_emptybase : emptybase { float a; };
+struct agg_float_emptybase pass_agg_float_emptybase(struct agg_float_emptybase arg) { return arg; }
+// CHECK-LABEL: define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, float %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z24pass_agg_float_emptybase19agg_float_emptybase(%struct.agg_float_emptybase* noalias sret align 4 %{{.*}}, i32 %{{.*}})
+struct noemptybasearray { [[no_unique_address]] empty dummy[3]; };
+struct agg_nofloat_emptybasearray : noemptybasearray { float a; };
+struct agg_nofloat_emptybasearray pass_agg_nofloat_emptybasearray(struct agg_nofloat_emptybasearray arg) { return arg; }
+// CHECK-LABEL: define void @_Z31pass_agg_nofloat_emptybasearray26agg_nofloat_emptybasearray(%struct.agg_nofloat_emptybasearray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+// SOFT-FLOAT-LABEL:  define void @_Z31pass_agg_nofloat_emptybasearray26agg_nofloat_emptybasearray(%struct.agg_nofloat_emptybasearray* noalias sret align 4 %{{.*}}, i64 %{{.*}})
+