clang::trivial_abi behaves differently on Windows compared to other Platforms

[tru]

See this example: https://godbolt.org/z/fT51az9qf

It seems like the calling convention makes the Windows output very verbose compared to the Linux one. The question is if this is intended or a bug somewhere.

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Tobias Hieta (tru)

See this example: https://godbolt.org/z/fT51az9qf

It seems like the calling convention makes the Windows output very verbose compared to the Linux one. The question is if this is intended or a bug somewhere.

[tru]

@rnk Any ideas? or do you know who might have ideas?

[tru]

Without the constructor it behaves as Linux.

[tru]

I have traced this to the following code in the clang codegen:

llvm-project/clang/lib/CodeGen/MicrosoftCXXABI.cpp

Line 1134 in ff9bc3a

for (const CXXConstructorDecl *Ctor : RD->ctors())

In the method MicrosoftCXXABI::isTrivialForMSVC it checks the following conditions:

  if (RD->hasProtectedFields() || RD->hasPrivateFields())
    return false;
  if (RD->getNumBases() > 0)
    return false;
  if (RD->isPolymorphic())
    return false;
  if (RD->hasNonTrivialCopyAssignment())
    return false;
  for (const CXXConstructorDecl *Ctor : RD->ctors())
    if (Ctor->isUserProvided())
      return false;
  if (RD->hasNonTrivialDestructor())
    return false;
  return true;

The condition that fails in my example above is that the code looks for any user provided Ctor's. This seems very broad and there are no similar checks for the Itanium ABI. Without too much knowledge of the Microsoft ABI, I wonder if the correct fix here is to check the triviality of the constructor instead of if there is a constructor at all maybe in combination with the trivial_abi attribute (but I am unsure of this since most other checks for the attribute is done in SemaDeclCXX, so it might be the wrong layer to check in codegen).

Any thoughts on this?

[tru]

CC @erichkeane @efriedma-quic

[efriedma-quic]

The MSVC rules are, as you note, asymmetric: in some cases, they allow passing a class directly, but not returning it directly.

I don't think there's any specific reason this code is in CodeGen rather than Sema.

It might make sense to allow some invented attribute to force the usage of Itanium-style rules for passing/returning a class in registers instead of the normal MSVC-style rules. I'm a little concerned about the ABI implications of modifying the meaning of trivial_abi at this point, but not sure how widely it's actually used.

[rnk]

The trivial_abi attribute feeds into canPassInRegisters which is checked at the call site above:

llvm-project/clang/lib/CodeGen/MicrosoftCXXABI.cpp

Line 1147 in ff9bc3a

bool isTrivialForABI = RD->canPassInRegisters() &&

The question I would ask is, why do we intersect canPassInRegisters and isTrivialForMSVC? I think that's the point where we say, "if its marked trivial abi, just use the platform-neutral canPassInRegisters property, otherwise, use the MSVC compatible logic". @dwblaikie was the last one to touch this MSVC-specific logic, not because he owns it, but because is hardworking and diligent. =D

---

Separately, I think it's probably time for LLVM to start assuming that sret values are returned in RAX, so that we can successfully tail call in sret cases like these:
https://godbolt.org/z/rsvbG88n6

LLVM only started guaranteeing this behavior partway through its history, so we didn't want to exploit this fact at that time, but it's been years since then.

[efriedma-quic]

The question I would ask is, why do we intersect canPassInRegisters and isTrivialForMSVC? I think that's the point where we say, "if its marked trivial abi, just use the platform-neutral canPassInRegisters property, otherwise, use the MSVC compatible logic".

MSVC ABI has different rules for passing arguments in registers, vs. returning values in registers. isTrivialForMSVC performs the checks that are specific to return values.

[rnk]

MSVC ABI has different rules for passing arguments in registers, vs. returning values in registers. isTrivialForMSVC performs the checks that are specific to return values.

Yep, I remember the painstaking updates to that routine to make it compatible with MSVC. The point you made, that I agree with, is that records carrying the trivial_abi attribute don't need to be passed ABI-compatibly with MSVC.

I guess we can early return true out of isTrivialForMSVC if RD->hasAttr<TrivialAbiAttr>() to fix this, with a comment that such records are not required to be MSVC ABI compatible.

However, note that later on the MSVC C++ ABI code forces all instance method struct returns to be done indirectly for more serious reasons, which will greatly limit the impact of this change. The ABI requires that instance methods always pass this as the first parameter, and that is used to produce thunks for member pointers and vtables without having complete return type information.

[tru]

I guess we can early return true out of isTrivialForMSVC if RD->hasAttr() to fix this, with a comment that such records are not required to be MSVC ABI compatible.

I put up a patch for this in #88857