[Bug d/110406] New: d: Wrong code-gen returning POD structs by value

[Bug d/110406] New: d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

            Bug ID: 110406
           Summary: d: Wrong code-gen returning POD structs by value
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: d
          Assignee: ibuclaw at gdcproject dot org
          Reporter: ibuclaw at gcc dot gnu.org
  Target Milestone: ---

Since 13.1 (r13-1104?), structs have been set the wrong mode, causing them to
return differently to equivalent static arrays.

---
struct cpuid_abcd_t
{
    uint eax;
    uint ebx;
    uint ecx;
    uint edx;
};

cpuid_abcd_t
cpuid_insn(const uint in_eax)
{
        cpuid_abcd_t ret = void;
        asm {
            "cpuid"
            :
                "=a" ( ret.eax ),
                "=b" ( ret.ebx ),
                "=c" ( ret.ecx ),
                "=d" ( ret.edx )
            :
                "a"  ( in_eax )
            :
    ;}
        return ret;
}

uint[4]
cpuid_insn2(const uint in_eax)
{
        uint[4] ret = void;
        asm {
            "cpuid"
            :
                "=a" ( ret[0] ),
                "=b" ( ret[1] ),
                "=c" ( ret[2] ),
                "=d" ( ret[3] )
            :
                "a"  ( in_eax )
            :
    ;}
        return ret;
}
---


The first function erroneously being returned in vector registers.

  _15 = {ret$eax_3, ret$ebx_4, ret$ecx_5, ret$edx_6};
  MEM <vector(4) uint> [(void *)&D.1894] = _15;
  return D.1894;

[Bug d/110406] d: Wrong code-gen returning POD structs by value

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Techincally the type mode should not be used by the backend to decide how a
struct is returned or not.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>structs have been set the wrong mode

No, they don't have wrong mode, just the x86_64 backend is broken, see bug
102027 comment #7 specifically.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

--- Comment #3 from ibuclaw at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #2)
> >structs have been set the wrong mode
> 
> No, they don't have wrong mode, just the x86_64 backend is broken, see bug
> 102027 comment #7 specifically.
I think they do.

On 64-bit,  I expect the mode set by compute_record_mode to be TImode.  However
what I'm instead seeing is BLKmode. This tells me I might be calling it too
early, and some fields yet to have their DECL_SIZE set.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

--- Comment #4 from ibuclaw at gcc dot gnu.org ---
It would be good if TYPE_MODE no longer had such a strong influence though.  In
the meantime, I ought to restore behaviour to how it was in 12.x

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to ibuclaw from comment #3)
> (In reply to Andrew Pinski from comment #2)
> > >structs have been set the wrong mode
> > 
> > No, they don't have wrong mode, just the x86_64 backend is broken, see bug
> > 102027 comment #7 specifically.
> I think they do.
> 
> On 64-bit,  I expect the mode set by compute_record_mode to be TImode. 
> However what I'm instead seeing is BLKmode. This tells me I might be calling
> it too early, and some fields yet to have their DECL_SIZE set.

Unless the struct has the alignment of TImode, it should be BLKmode ...

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |ABI

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #5)
> (In reply to ibuclaw from comment #3)
> > (In reply to Andrew Pinski from comment #2)
> > > >structs have been set the wrong mode
> > > 
> > > No, they don't have wrong mode, just the x86_64 backend is broken, see bug
> > > 102027 comment #7 specifically.
> > I think they do.
> > 
> > On 64-bit,  I expect the mode set by compute_record_mode to be TImode. 
> > However what I'm instead seeing is BLKmode. This tells me I might be calling
> > it too early, and some fields yet to have their DECL_SIZE set.
> 
> Unless the struct has the alignment of TImode, it should be BLKmode ...

here is a C testcase to get it returned into vector register (incorrectly due
to the alignment being set to 32):
```
typedef unsigned uint;
struct  cpuid_abcd_t
{
    uint eax;
    uint ebx;
    uint ecx;
    uint edx;
}  __attribute__((aligned(8*4)));

struct cpuid_abcd_t
cpuid_insn(const uint in_eax)
{
        struct cpuid_abcd_t ret={};

        asm (
            "cpuid"
            :
                "=a" ( ret.eax ),
                "=b" ( ret.ebx ),
                "=c" ( ret.ecx ),
                "=d" ( ret.edx )
            :
                "a"  ( in_eax )
            :
    );
        return ret;
}
```

Which itself is GCC 12+ regression too ...

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[pinskia at gcc dot gnu.org]

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

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #6)
> Which itself is GCC 12+ regression too ...

I filed that as PR 110407, let's see what the x86 backend folks say ...

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

--- Comment #8 from ibuclaw at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #7)
> (In reply to Andrew Pinski from comment #6)
> > Which itself is GCC 12+ regression too ...
> 
> I filed that as PR 110407, let's see what the x86 backend folks say ...
OK, in the meantime I will proceed with seeing what I did to change the return
behaviour on my end.

So long as C and D agree with each other when it comes to POD types, it almost
doesn't matter to me weather the back-end is following the wrong ABI.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

--- Comment #9 from ibuclaw at gcc dot gnu.org ---
(In reply to ibuclaw from comment #8)
> So long as C and D agree with each other when it comes to POD types, it
> almost doesn't matter to me weather the back-end is following the wrong ABI.
Or whether.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

--- Comment #10 from ibuclaw at gcc dot gnu.org ---
Looking at an ICE in stage2 D compiler for i686-darwin17, I realized that it's
another facet of this issue.

// aggregate.d
import dsymbol;
extern (C++) class AggregateDeclaration : ScopeDsymbol
{
    Visibility visibility;
    override Visibility visible() { return visibility; }
}



// dsymbol.d
struct Visibility
{
    enum Kind { undefined }
    Kind kind;
}
extern(C++) class ScopeDsymbol
{
    Visibility visible();
    Visibility.Kind* visibilities; 
}


When compiling dsymbol.d, `struct Visibility` is set SImode.  However when
compiling aggregate.d it's instead given BLKmode.  This causes all the
difference in how it is returned.  Any C++ code interfacing with this will
crash due to the mismatch too.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[iains at gcc dot gnu.org]

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

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |iains at gcc dot gnu.org

--- Comment #11 from Iain Sandoe <iains at gcc dot gnu.org> ---
If I remember correctly, the underlying issue is that D always has a vtable
pointer for a "class" whereas C++ only adds one if needed (i.e. there are
actual virtual methods)

So we really need to use the 'struct' tag to D for classes without virtual
methods that need to interoperate with C++.  I think that then D will lay them
out without the vtable pointer.

We had a fix for this for Darwin - which does not seem to have made upstream
just yet.  Restesting (there's an unrelated bootstrap regression to work
around).

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[iains at gcc dot gnu.org]

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

--- Comment #12 from Iain Sandoe <iains at gcc dot gnu.org> ---
OTOH there was a second issue with zero-sized objects which was fixed thus:

diff --git a/gcc/d/types.cc b/gcc/d/types.cc
index a1f69bb02b7..020cc7de83f 100644
--- a/gcc/d/types.cc
+++ b/gcc/d/types.cc
@@ -581,6 +581,11 @@ finish_aggregate_mode (tree type)
 {
   for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
     {
+      /* Fields of type `typeof(*null)' have no size, so let them force the
+        record type mode to be computed as BLKmode.  */
+      if (TYPE_MAIN_VARIANT (TREE_TYPE (field)) == noreturn_type_node)
+       break;
+
       if (DECL_SIZE (field) == NULL_TREE)
        return;
     }

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

ibuclaw at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-06-28
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |ibuclaw at gcc dot gnu.org

--- Comment #13 from ibuclaw at gcc dot gnu.org ---
Created attachment 55413
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55413&action=edit
delay calling compute_record_mode until all fields complete

Attaching the full patch that was being tested, related to the above snippet.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[cvs-commit at gcc dot gnu.org]

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

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain Buclaw <ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:c201cd3be0d9ab887fafb0c33a9fc287c405c21c

commit r14-2169-gc201cd3be0d9ab887fafb0c33a9fc287c405c21c
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Wed Jun 28 18:30:31 2023 +0200

    d: Fix wrong code-gen when returning structs by value.

    Since r13-1104, structs have have compute_record_mode called too early
    on them, causing them to return differently depending on the order that
    types are generated in, and whether there are forward references.

    This patch moves the call to compute_record_mode into its own function,
    and calls it after all fields have been given a size.

            PR d/106977
            PR target/110406

    gcc/d/ChangeLog:

            * types.cc (finish_aggregate_mode): New function.
            (finish_incomplete_fields): Call finish_aggregate_mode.
            (finish_aggregate_type): Replace call to compute_record_mode with
            finish_aggregate_mode.

    gcc/testsuite/ChangeLog:

            * gdc.dg/torture/pr110406.d: New test.

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Iain Buclaw
<ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:f2eeda5652438fe783d4e3878139481a1b8606b6

commit r13-7496-gf2eeda5652438fe783d4e3878139481a1b8606b6
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Wed Jun 28 18:30:31 2023 +0200

    d: Fix wrong code-gen when returning structs by value.

    Since r13-1104, structs have have compute_record_mode called too early
    on them, causing them to return differently depending on the order that
    types are generated in, and whether there are forward references.

    This patch moves the call to compute_record_mode into its own function,
    and calls it after all fields have been given a size.

            PR d/106977
            PR target/110406

    gcc/d/ChangeLog:

            * types.cc (finish_aggregate_mode): New function.
            (finish_incomplete_fields): Call finish_aggregate_mode.
            (finish_aggregate_type): Replace call to compute_record_mode with
            finish_aggregate_mode.

    gcc/testsuite/ChangeLog:

            * gdc.dg/torture/pr110406.d: New test.

    (cherry picked from commit c201cd3be0d9ab887fafb0c33a9fc287c405c21c)

[Bug target/110406] d: Wrong code-gen returning POD structs by value

[ibuclaw at gcc dot gnu.org]

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

ibuclaw at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #16 from ibuclaw at gcc dot gnu.org ---
Fixed and backported problem specific to the D front-end.  There's another part
to this in pr102027, but I don't think this should be kept open waiting for it.