[Bug c++/105260] New: Union with user-defined empty destructor leads to worse code-gen

[Bug c++/105260] New: Union with user-defined empty destructor leads to worse code-gen

[m.cencora at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

            Bug ID: 105260
           Summary: Union with user-defined empty destructor leads to
                    worse code-gen
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: m.cencora at gmail dot com
  Target Milestone: ---

Following code leads to unnecessary stack spill of deserialized Foo variable:
g++ -std=c++17 -O2

#include <new>

inline unsigned deserializeUInt(const unsigned char* &in)
{
    unsigned out;
    __builtin_memcpy(&out, in, sizeof(out));
    in += sizeof(out);
    out = __builtin_bswap32(out);
    return out;
}

struct Foo
{
    unsigned a;
    unsigned b;

    static Foo deserialize(const unsigned char* &in)
    {
        return Foo{
            deserializeUInt(in),
            deserializeUInt(in)
        };
    }
};

struct Result
{
    unsigned idx;
    union
    {
        unsigned a;
        const void* ptr;
    };
};

Result dummyFunc(Foo);

void deserializeAndInvoke(const unsigned char* it)
{
#ifndef WORKAROUND
    union NoDestroy
    {
        ~NoDestroy() {}
        Foo value;
    };

    NoDestroy un{ 
        Foo::deserialize(it)
    };
    auto& arg = un.value;

#elif WORKAROUND == 1
    union NoDestroy
    {
        Foo value;
    };
    NoDestroy un{ 
        Foo::deserialize(it)
    };
    auto& arg = un.value;

#elif WORKAROUND == 2  
    alignas(Foo) char rawStorage[sizeof(Foo)];

    auto& arg = *new (&rawStorage[0]) Foo{
        Foo::deserialize(it)
    };
#endif

    dummyFunc(arg);
}

deserializeAndInvoke(unsigned char const*):
        mov     edx, DWORD PTR [rdi]
        mov     eax, DWORD PTR [rdi+4]
        bswap   edx
        bswap   eax
        mov     DWORD PTR [rsp-16], edx
        mov     DWORD PTR [rsp-12], eax
        mov     rdi, QWORD PTR [rsp-16]
        jmp     dummyFunc(Foo)

The spill can be avoided, when we remove user-defined destructor of NoDestroy
union, or
when we construct Foo via placement new in raw buffer.

Generated code with either of the workarounds:
g++ -std=c++17 -O2 -DWORKAROUND=1
or
g++ -std=c++17 -O2 -DWORKAROUND=2

deserializeAndInvoke(unsigned char const*):
        mov     rax, rdi
        mov     edi, DWORD PTR [rdi]
        mov     eax, DWORD PTR [rax+4]
        bswap   edi
        mov     edi, edi
        bswap   eax
        sal     rax, 32
        or      rdi, rax
        jmp     dummyFunc(Foo)

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Well, it simply looks like the calling conventions for dummyFunc are different
with the ABI in effect depending on the PODness(?), and the code is as
optimized as it can be.

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |ABI, wrong-code

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
OTOH NoDestroy is not really used but affects 'Foo'?

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
That said, 'auto& arg' might just hide the interesting bit but my C++ fu is too
weak to see how un.value would differ.

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[m.cencora at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

--- Comment #4 from m.cencora at gmail dot com ---
I dont think ABI is an issue here. The Foo variable is spilled into stack, and
then reloaded back into RDI register before invoking dummyFunc.
Also clang generates optimal code as can be seen here:
https://godbolt.org/z/K6jWY9h3b

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[m.cencora at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

--- Comment #5 from m.cencora at gmail dot com ---
I've slighlty refactored the code, to remove the auto variables. This issue
remains

#include <new>

inline unsigned deserializeUInt(const unsigned char* &in)
{
    unsigned out;
    __builtin_memcpy(&out, in, sizeof(out));
    in += sizeof(out);
    out = __builtin_bswap32(out);
    return out;
}

struct Foo
{
    unsigned a;
    unsigned b;

    static Foo deserialize(const unsigned char* &in)
    {
        return Foo{
            deserializeUInt(in),
            deserializeUInt(in)
        };
    }
};

struct Result
{
    unsigned idx;
    union
    {
        unsigned a;
        const void* ptr;
    };
};

Result dummyFunc(Foo);

void deserializeAndInvoke(const unsigned char* it)
{
#ifndef WORKAROUND
    union NoDestroy
    {
        ~NoDestroy() {}
        Foo value;
    };

    NoDestroy un{ 
        Foo::deserialize(it)
    };
    dummyFunc(un.value);

#elif WORKAROUND == 1
    union NoDestroy
    {
        Foo value;
    };
    NoDestroy un{ 
        Foo::deserialize(it)
    };
    dummyFunc(un.value);

#elif WORKAROUND == 2  
    alignas(Foo) char rawStorage[sizeof(Foo)];

    Foo* foo = new (&rawStorage[0]) Foo{
        Foo::deserialize(it)
    };
    dummyFunc(*foo);
#endif

}

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[m.cencora at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

--- Comment #6 from m.cencora at gmail dot com ---
Furthermore in all the scenarios the same function is called, with same
arguments, so the calling convention/ABI is same.

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |jamborm at gcc dot gnu.org
           Keywords|ABI, wrong-code             |

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I don't see any ABI differences, nor wrong-code.
If you look at *.optimized dump differences, there are pretty much none, except
for one extra CLOBBER.
But there are 2 differences not visible in the dump.
One is that while neither un variable is TREE_ADDRESSABLE, the no WORKAROUND
one has TREE_ADDRESSABLE type (as it has non-trivial destructor).
The other difference that matters for this testcase is that TYPE_MODE of the
NoDestroy union type is BLKmode, while for WORKAROUND=1 case it is DImode.

This is both done in:
  /* If this type has a copy constructor or a destructor, force its
     mode to be BLKmode, and force its TREE_ADDRESSABLE bit to be
     nonzero.  This will cause it to be passed by invisible reference
     and prevent it from being returned in a register.  */
  if (type_has_nontrivial_copy_init (t)
      || TYPE_HAS_NONTRIVIAL_DESTRUCTOR (t))
    {
      tree variants;
      SET_DECL_MODE (TYPE_MAIN_DECL (t), BLKmode);
      for (variants = t; variants; variants = TYPE_NEXT_VARIANT (variants))
        {
          SET_TYPE_MODE (variants, BLKmode);
          TREE_ADDRESSABLE (variants) = 1;
        }
    }
Now, for the ABI passing the above is really required, such types are indeed
passed differently from ones that have trivialy copy ctor and dtor, but for
this testcase it is just an optimization decision (use_register_for_decl).
We do there:
  /* Only register-like things go in registers.  */
  if (DECL_MODE (decl) == BLKmode)
    return false;
Guess we'd need to recompute mode for non-TREE_ADDRESSABLE vars before
expansion.
Or find out why SRA doesn't optimize this (remove the useless union, replace
all the un.value occurrences with a var with Foo type.

[Bug c++/105260] Union with user-defined empty destructor leads to worse code-gen

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105260

--- Comment #8 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #7)
> Or find out why SRA doesn't optimize this (remove the useless union, replace
> all the un.value occurrences with a var with Foo type.

IIUC, it just isn't profitable, SRA sees:

  un.value = D.2545;
  dummyFunc (MEM[(const struct Foo &)&un]);
  un ={v} {CLOBBER(eol)};

and it just cannot avoid ultimately setting up the structure and pass
it to dummyFunc (IPA-SRA does not control that function and so cannot
change it).