This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by uweigand on Jun 10 2020, 9:12 AM.

Details

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 ...

Diff Detail

Event Timeline

uweigand created this revision.Jun 10 2020, 9:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 9:12 AM

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

lkail added a subscriber: lkail.Jul 7 2020, 5:37 AM
hubert.reinterpretcast added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
537

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

if (isa<CXXRecordDecl>(RT->getDecl()) &&
    !(AllowNoUniqueAddr && FD->hasAttr<NoUniqueAddressAttr>()))
  return false;
uweigand updated this revision to Diff 276151.Jul 7 2020, 11:26 AM

Rebased against mainline.

uweigand marked an inline comment as done.Jul 7 2020, 11:26 AM

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
528

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

7251–7257

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?

clang/lib/CodeGen/TargetInfo.cpp
525

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

7237

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.

7255

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

qiucf added a subscriber: qiucf.Jul 7 2020, 8:24 PM

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 updated this revision to Diff 276414.Jul 8 2020, 6:59 AM

Handle array of empty records correctly.

uweigand marked 6 inline comments as done.Jul 8 2020, 7:07 AM
uweigand added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
525

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

528

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

7251–7257

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 marked 2 inline comments as done.Jul 8 2020, 7:08 AM

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)?

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 added a comment.Jul 8 2020, 9:14 AM

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)

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.

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
7251–7257

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 updated this revision to Diff 276650.Jul 9 2020, 12:50 AM

Drop AllowNoUniqueAddress parameter; address review comment.

uweigand marked 2 inline comments as done.Jul 9 2020, 12:54 AM
asb added a comment.Jul 9 2020, 4:40 AM

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 accepted this revision.Jul 9 2020, 4:16 PM

LGTM

This revision is now accepted and ready to land.Jul 9 2020, 4:16 PM