[Bug tree-optimization/109543] New: Avoid using BLKmode for unions with a non-BLKmode member when possible

[Bug tree-optimization/109543] New: Avoid using BLKmode for unions with a non-BLKmode member when possible

[avieira at gcc dot gnu.org]

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

            Bug ID: 109543
           Summary: Avoid using BLKmode for unions with a non-BLKmode
                    member when possible
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: avieira at gcc dot gnu.org
  Target Milestone: ---

Hi,

So with the following C-code:
$ cat t.c
#include <arm_neon.h>
#ifdef GOOD
typedef float64x2x2_t TYPE;
#else
typedef union
{
      float64x2x2_t v;
      double d[4];
  } TYPE;
  #endif


void foo (TYPE *a, TYPE *b, TYPE *c, unsigned  n)
{
 TYPE X = a[0];
 TYPE Y = b[0];
 TYPE Z = c[0];
 for (unsigned  i = 0; i < n; ++n)
 {
  TYPE temp = X;
  X = Y;
  Y = Z;
  Z = temp;
 }
 a[0] = X;
 b[0] = Y;
 c[0] = Z;
}

If compiled for aarch64 with -DGOOD the compiler will use vector register moves
in the loop, whereas without -DGOOD it will use the stack with memmoves.

The reason for this is because when picking the mode to address a UNION with
gcc will always choose BLKmode as soon as any member of a UNION is BLKmode. In
such cases I think it would be safe to go with not-BLKmode of members that have
the same size as the entire UNION?

[Bug tree-optimization/109543] Avoid using BLKmode for unions with a non-BLKmode member when possible

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-04-18
                 CC|                            |jamborm at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
A safe mode would have to be an integer mode supporting bitwise copying. 
Usually SRA is responsible for selecting "register-like" types for aggregates
but it might in this case stay away because of the union.

But it really looks like a classy example SRA should handle (for example
by splitting TYPE into four DImode integers - it wouldn't currently
chose vectors but of course it eventually could).

Can you adjust the testcase to use GCC generic vectors?

[Bug tree-optimization/109543] Avoid using BLKmode for unions with a non-BLKmode member when possible

[avieira at gcc dot gnu.org]

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

--- Comment #2 from avieira at gcc dot gnu.org ---
Sorry for the delay. Here's the typedefs with GNU vectors.  

typedef struct 
{
    float __attribute__ ((vector_size(16))) v[2];
} STRUCT;

#ifdef GOOD
typedef STRUCT TYPE;
#else
typedef union
{
    STRUCT s;
    double d[2];
} TYPE;
#endif

To be fair I suspect you could see similar behaviour with just 16-byte vectors,
but with aarch64 the backend will know to use 64-bit scalar moves for 128-bit
BLKmodes, though even then, picking the vector mode would result in more
optimal (single vector move) code.

[Bug tree-optimization/109543] Avoid using BLKmode for unions with a non-BLKmode member when possible

[avieira at gcc dot gnu.org]

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

--- Comment #3 from avieira at gcc dot gnu.org ---
Err that should be 'double d[4];' so:
typedef struct 
{
    float __attribute__ ((vector_size(16))) v[2];
} STRUCT;

#ifdef GOOD
typedef STRUCT TYPE;
#else
typedef union
{
    STRUCT s;
    double d[4];
} TYPE;
#endif

[Bug tree-optimization/109543] Avoid using BLKmode for unions with a non-BLKmode member when possible

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think for SRA the issue is that there's no access to any of the unions
components and as Martin says it disqualifies total scalarization.

I think total scalarization would be possible if we can use the one and
only component type that's actually used (and that component is a register
type) or if there's no component explicitely used and we have a register
that's suitable to represent the whole bit representation of the union.

At materialization time the only thing we have to make sure is that we
not replace

 a[0] = X;

with

 a[0].component = X-component_register;

but retain the union typed copy like

 a[0] = V_C_E <union-type> (X-component_register);

(not sure if that's valid GIMPLE ...).  The important thing is to
preserve the TBAA behavior of both the original aggregate loads and
the store which means we have to keep the union type visible in the
access path, not only preserving the alias-set (at least I think so).

That said, a vector-float isn't a good bit representation.  There's
eventually type_for_mode () using the union mode if it's not BLKmode.
That might not be the worst choice.

[Bug tree-optimization/109543] Avoid using BLKmode for unions with a non-BLKmode member when possible

[rsandifo at gcc dot gnu.org]

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

--- Comment #5 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
I think the loop in compute_mode_layout needs to be smarter
for unions.  At the moment it's sensitive to field order,
which doesn't make much conceptual sense.

E.g. for the admittedly contrived example:

#include <arm_neon.h>

union u1 {
  int32x2x2_t x;
  __int128 y __attribute__((packed));
};

union u2 {
  __attribute__((packed)) __int128 y;
  int32x2x2_t x;
};

compiled with -mstrict-align, the loop produces V2x2SImode for
union u1 (good!) but TImode for union u2 (requires too much alignment).
That doesn't matter as things stand, since we don't accept unions
with vector modes.  But if we did, union u1 would be placed in registers
and union u2 wouldn't.

[Bug tree-optimization/109543] Avoid using BLKmode for unions with a non-BLKmode member when possible

[richard.sandiford at arm dot com]

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

--- Comment #6 from richard.sandiford at arm dot com ---
Here's a proof of concept patch that fixes the testcase for
-mstrict-align.  The VECTOR_MODE_P part would need to be behind
a new target hook, to avoid accidentally breaking someone's ABI.

For default-align, we should probably make double[4] use V4DFmode
via aarch64_array_mode.

[Bug tree-optimization/109543] Avoid using BLKmode for unions with a non-BLKmode member when possible

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Yeah, guess we still have target code doing ABI decisions based on mode ...