llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.86k stars 11.48k forks source link

[RISC-V] Empty structs are ignored when passing in C++ #97285

Closed tomeksowi closed 2 months ago

tomeksowi commented 2 months ago

According to RISC-V integer calling convention:

Empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types.

struct Empty {};

int after_empty(int i0, Empty e, int i1)
{
    return i1;
}

int after_empty_on_stack(
    int i0, int i1, int i2, int i3, int i4, int i5, int i6, int i7, Empty e, int i8)
{
    return i8;
}

Clang doesn't assign a register nor a stack slot to the empty struct for passing:

after_empty(int, Empty, int):                # @after_empty(int, Empty, int)
        mv      a0, a1
        ret
after_empty_on_stack(int, int, int, int, int, int, int, int, Empty, int): # @after_empty_on_stack(int, int, int, int, int, int, int, int, Empty, int)
        ld      a0, 0(sp)
        ret

I'd expect these functions to be mv a0, a2 and ld a0, 8(sp) respectively.

This also doesn't match GCC which does allocate a register and stack slot to the empty struct argument. Comparison with GCC on Godbolt

ISA: rv64gc Clang version: 18.1.0 and Godbolt "trunk" at the time of filing this issue (2024-07-01).

tomeksowi commented 2 months ago

Part of dotnet/runtime#84834

llvmbot commented 2 months ago

@llvm/issue-subscribers-backend-risc-v

Author: Tomasz Sowiński (tomeksowi)

According to RISC-V integer calling convention: > Empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. **This is not the case for C++, which requires them to be sized types.** ```C++ struct Empty {}; int after_empty(int i0, Empty e, int i1) { return i1; } int after_empty_on_stack( int i0, int i1, int i2, int i3, int i4, int i5, int i6, int i7, Empty e, int i8) { return i8; } ``` Clang doesn't assign a register nor a stack slot to the empty struct for passing: ```asm after_empty(int, Empty, int): # @after_empty(int, Empty, int) mv a0, a1 ret after_empty_on_stack(int, int, int, int, int, int, int, int, Empty, int): # @after_empty_on_stack(int, int, int, int, int, int, int, int, Empty, int) ld a0, 0(sp) ret ``` I'd expect these functions to be `mv a0, a2` and `ld a0, 8(sp)` respectively. This also doesn't match GCC which does allocate a register and stack slot to the empty struct argument. [Comparison with GCC on Godbolt](https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXLnwEEAqgGdMABQAenAAy8AVhNK0mDUMgCkAJgBCZ86UX1kBPAMqN0AYVS0AriwYhTpFwAZPAZMADlvACNMYgl/AAdUeUJHBncvH11E5IcBYNCIlmjYqVtMe1ShAiZiAnTvX38yioEqmoJ88KiYuJtq2vrM0v6OkK6inq4AShtUT2Jkdg55AmJPewBqbBZ4ggBPDeMAdksjgBFjAGZLdQBBYzuQgg2mKgIYgH1MHf2IJ428OpSFsfgdMMD/nhpg97icYQ8AJzETAEeYMAFcK43WEXO4w/6vd7EL6gj4CD4rJjIADWEBhGwZAMEAKBTOeUIhzLw/jZAMunPZABYBQCAKwivAANglR2B212YIlAA4pvC4Xj1EiUWiAUqsWqzhwZrROKLeL5uLxUJxXFYrBt5HMFphDqZLjxSAQNEaZtSQJdLgA6AMh0Nh6UmjiC83e0jWji8eQgIFejiaGZwWBINA7OgxciUHPxPOxYgAN0lgoAtMh9IYuEquECaLQiUmIJFY5EQjU9pwPd3mMQ9gB5SJacqpj05tiCEcMWh9tO8LCRTzAVxiWhJy2kLAsAzAcTLvd4ZEVMuYHeaAIqcqed793hPTCRm%2B0PCRYi99xYWOrPAWCfUhL2ISIkkwM5vkPD9DG9GYqH0YB5AANTwTAAHcR3iRhgP4QQRDEdhJGkQQFGUNQTx0KQ6yMO0LD0T8k0gGZUF2VIdyrEcAC8rVA4g8CwZiIBmZpcl8CAXEGXwuH8IIxkKYoQGlbIUgEaTelU8TOkUnppTEyoRg02SbFfSdDPaHTulifSjI8BpekpWorImGzRKdRYJGNU0YxPeMNgAJQASSEVwqxQjZy0rYBkA2WtDw2BtAy4QN1A2CBcEIEhXXdKZeFTdNfX9IMw1KkMI04aNSAtG940TZNPXg0hM0QFBUFzegyAoCAixLEAourGLkC4QUmz4Og20oTsT0HXtgNm4cxwnexgJnRgCHnRdY1XddN1obdgP3GClhvfBzwcS9r14TA72QB8lg9F8314D8vx/DATvygSgN3UDwMUKCD0MWDQGXBCkNQ9CsJwi0PXw4RRHEEj4fI1RYx0fxaJQejrFe4TWPYgROJ4viYgEoT4FEsyWgkqT7KGAIGHQFylKkLTUmM0h2byBTrN0AzWjsjIZNMuxxLaZzedc/mhYc4ZLKl1n3PmTzpj0HzqtjfzgtC8L0srKtuwIeQpg2IbEsFZLUvSzKiGIHLpny%2BCioDYMytK9Wo182rOHqlNnc90xvatX3GrBmZQOSJxBSAA%3D%3D) ISA: rv64gc Clang version: 18.1.0 and Godbolt "trunk" at the time of filing this issue (2024-07-01).
svs-quic commented 2 months ago

The following change seems to fix the issue:

--- a/clang/lib/CodeGen/Targets/RISCV.cpp
+++ b/clang/lib/CodeGen/Targets/RISCV.cpp
@@ -361,12 +361,12 @@ ABIArgInfo RISCVABIInfo::classifyArgumentType(QualType Ty, bool IsFixed,
                                            CGCXXABI::RAA_DirectInMemory);
   }

+  uint64_t Size = getContext().getTypeSize(Ty);
+
   // Ignore empty structs/unions.
-  if (isEmptyRecord(getContext(), Ty, true))
+  if (isEmptyRecord(getContext(), Ty, true) && Size == 0)
     return ABIArgInfo::getIgnore();

-  uint64_t Size = getContext().getTypeSize(Ty);
-
   // Pass floating point values via FPRs if possible.
   if (IsFixed && Ty->isFloatingType() && !Ty->isComplexType() &&
       FLen >= Size && ArgFPRsLeft) {
tomeksowi commented 2 months ago

@svs-quic Looks like the same problem exists on 32-bit ARM. Comparison against GCC on Godbolt

I'm less familiar with ARM 32 so I can't point to the exact wording in the ABI that specifies empty struct treatment.

svs-quic commented 2 months ago

@svs-quic Looks like the same problem exists on 32-bit ARM. Comparison against GCC on Godbolt

I'm less familiar with ARM 32 so I can't point to the exact wording in the ABI that specifies empty struct treatment.

Same here. Perhaps @efriedma-quic might be some idea?