[Bug target/57949] New: [powerpc64] Structure parameter alignment issue with vector extensions

[Bug target/57949] New: [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

            Bug ID: 57949
           Summary: [powerpc64] Structure parameter alignment issue with
                    vector extensions
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Keywords: ABI, wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: wschmidt at gcc dot gnu.org
                CC: bergner at vnet dot ibm.com, dje at gcc dot gnu.org
              Host: powerpc64-*-*
            Target: powerpc64-*-*
             Build: powerpc64-*-*

Created attachment 30533
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30533&action=edit
Generated assembly with -O2 -maltivec

When passing parameters, the 64-bit PowerPC ELF ABI requires:

"Fixed size aggregates and unions passed by value are mapped to as many
doublewords of the parameter save area as the value uses in memory.  Aggregates
and unions are aligned according to their alignment requirements.  This may
result in doublewords being skipped for alignment."

The following demonstrates a case where a structure is currently misaligned in
the parameter save area:

=====
typedef float v4sf __attribute__ ((vector_size (16)));

struct s { long m; v4sf v; };

extern long n;
extern v4sf ve;

void test3 (long d1, long d2, long d3, long d4, long d5, long d6,
            long d7, long d8, long d9, struct s vs) {
  n = vs.m;
  ve = vs.v;
}
=====

Member v of struct s has 16-byte alignment, requiring internal padding of 8
bytes between members m and v.  This is correctly honored.  A struct s should
have 16-byte alignment because it contains a member requiring 16-byte
alignment.  When passing parameters, this apparently is not being honored.

The parameter save area begins at 48(r1).  Accordingly, d9 should be placed at
112(r1).  Alignment padding should require vs to begin at 128(r1).  So vs.m
should be at 128(r1), and vs.v should be at 144(r1).

The attached assembly code (compiled with -O2 -maltivec on
powerpc64-unknown-linux-gnu) shows that vs.m is accessed at 120(r1) and vs.v is
accessed at 136(r1).

If vs.v were to be accessed as a vector with an lvx instruction, we would get
incorrect code because the low-order 4 bits of the address would be ignored,
causing a load from 128(r1).  However, there appears to be code that avoids
this problem by copying the vector member to an aligned stack slot using ld/std
before loading it as a vector.

There is not a problem with local stack variables; so far as I can tell, a
local "struct s" is always aligned correctly.

It's not clear to me without further investigation whether this is a problem
unique to PPC64 or whether it could occur on other targets.  Opening this as a
target-specific bug for now.

Note that we may not be able to change this behavior at this time.  Fixing it
would cause incompatibilities with existing libraries that pass such
structures.  However, such interfaces are probably rare.  It may be worth
fixing this and providing an option to compile with the old behavior, in case
anyone runs into an incompatibility.

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

--- Comment #1 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
The front end identifies the structure as having the correct alignment.  From
the 001t.tu dump:

@2846   record_type      name: @2857    size: @127     algn: 128     
                         tag : struct   flds: @2858   
@2857   identifier_node  strg: s        lngt: 1       
@2858   field_decl       name: @2870    type: @16      scpe: @2846   
                         srcp: vec-abi.c:3             chain: @2871   
                         size: @22      algn: 64       bpos: @20     
@2870   identifier_node  strg: m        lngt: 1       
@2871   field_decl       name: @2882    type: @2817    scpe: @2846   
                         srcp: vec-abi.c:3             size: @19     
                         algn: 128      bpos: @19     
@2882   identifier_node  strg: v        lngt: 1

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

--- Comment #2 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
The wrong code is introduced during expand.  vs.m is computed as

  mem(plus(virtual-incoming-args, 72))

with the pad at offset 80, v[0..1] at offset 88, and v[2..3] at offset 96.  All
are shown as having alignment 8.  vs.m should have been placed at offset 80.

After loading these into virtual regs with DI mode, they are stored at offsets
0, 8, 16, 24 from virtual-stack-vars, which is given an alignment of 128. 
Later the vector is loaded with V4SF mode.

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

--- Comment #4 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Enabling the code used for MachO/Darwin64 when targeting ABI_AIX/linux produces
much better code:

    li 9,144
    addis 8,2,.LC1@toc@ha
    lvx 0,1,9
    ld 10,.LC1@toc@l(8)
    addis 8,2,.LC2@toc@ha
    ld 9,.LC2@toc@l(8)
    ld 8,128(1)
    stvx 0,0,9
    std 8,0(10)
    blr

A properly aligned vector load is used from the parameter save area without a
copy through the local variable area.

David/Peter, two questions:

(1) Would it be reasonable to make this the new default behavior, but add a
target-specific flag (-munaligned-vect-struct-parms or something) to allow
compatibility with the existing behavior?  I doubt this comes up often, but it
probably occurs somewhere in an existing library interface.

(2) If we make a change, should it be just for Linux, or should we change code
for AIX as well?

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |joseph at codesourcery dot com

--- Comment #5 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Joseph, adding you for any comments you might have relative to Freescale
impacts.

Summarizing some offline discussion:
 * This would apply to long double, __int128, and (probably) _Decimal128 as
well.  The fix is the same in all cases.
 * There may be some oddities for _Decimal128, as the need to be in even/odd
register pairs may conflict with alignment in storage.  The 64-bit ABI does not
currently specify the alignment for _Decimal128 so we'll need to check the code
to see what's the case in practice.

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I'm not expert on the 64-bit ABI; the Power.org ABI TSC never really got 
onto doing anything with the 64-bit ABI, although nominally it's in scope.  
The only Freescale peculiarity I know of for 64-bit processors is that 
some don't have fsqrt, but that's nothing to do with the ABI.

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

--- Comment #7 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
I rewrote the test case to use the IBM vector extensions and ran it through
xlc.  The generated code shows that xlc addresses the code as expected by the
ABI (and contrary to what's done by gcc).  So this adds weight to the argument
that gcc should change its default behavior, and provide an option to generate
code for the old behavior when necessary.

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

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

--- Comment #8 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Fixed as r201750.

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[uweigand at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

--- Comment #9 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Author: uweigand
Date: Fri Nov 15 23:39:50 2013
New Revision: 204870

URL: http://gcc.gnu.org/viewcvs?rev=204870&root=gcc&view=rev
Log:
gcc:

2013-11-15  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>

    Backport from mainline r201750.
    Note: Default setting of -mcompat-align-parm inverted!

    2013-08-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

    PR target/57949
    * doc/invoke.texi: Add documentation of mcompat-align-parm
    option.
    * config/rs6000/rs6000.opt: Add mcompat-align-parm option.
    * config/rs6000/rs6000.c (rs6000_function_arg_boundary): For AIX
    and Linux, correct BLKmode alignment when 128-bit alignment is
    required and compatibility flag is not set.
    (rs6000_gimplify_va_arg): For AIX and Linux, honor specified
    alignment for zero-size arguments when compatibility flag is not
    set.

gcc/testsuite:

2013-11-15  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>

    Backport from mainline r201750.
    Note: Default setting of -mcompat-align-parm inverted!

    2013-08-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

    PR target/57949
    * gcc.target/powerpc/pr57949-1.c: New.
    * gcc.target/powerpc/pr57949-2.c: New.


Added:
    branches/ibm/gcc-4_8-branch/gcc/testsuite/gcc.target/powerpc/pr57949-1.c
    branches/ibm/gcc-4_8-branch/gcc/testsuite/gcc.target/powerpc/pr57949-2.c
Modified:
    branches/ibm/gcc-4_8-branch/gcc/ChangeLog.ibm
    branches/ibm/gcc-4_8-branch/gcc/config/rs6000/rs6000.c
    branches/ibm/gcc-4_8-branch/gcc/config/rs6000/rs6000.opt
    branches/ibm/gcc-4_8-branch/gcc/doc/invoke.texi
    branches/ibm/gcc-4_8-branch/gcc/testsuite/ChangeLog.ibm

[Bug target/57949] [powerpc64] Structure parameter alignment issue with vector extensions

[wschmidt at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57949

--- Comment #10 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Author: wschmidt
Date: Fri Apr  4 14:05:08 2014
New Revision: 209095

URL: http://gcc.gnu.org/viewcvs?rev=209095&root=gcc&view=rev
Log:
[gcc]

2014-04-04  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

    Backport from mainline r201750.
    Note: Default setting of -mcompat-align-parm inverted!

    2013-08-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

    PR target/57949
    * doc/invoke.texi: Add documentation of mcompat-align-parm
    option.
    * config/rs6000/rs6000.opt: Add mcompat-align-parm option.
    * config/rs6000/rs6000.c (rs6000_function_arg_boundary): For AIX
    and Linux, correct BLKmode alignment when 128-bit alignment is
    required and compatibility flag is not set.
    (rs6000_gimplify_va_arg): For AIX and Linux, honor specified
    alignment for zero-size arguments when compatibility flag is not
    set.

[gcc/testsuite]

2014-04-04  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

    Backport from mainline r201750.
    Note: Default setting of -mcompat-align-parm inverted!

    2013-08-14  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

    PR target/57949
    * gcc.target/powerpc/pr57949-1.c: New.
    * gcc.target/powerpc/pr57949-2.c: New.




Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.target/powerpc/pr57949-1.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.target/powerpc/pr57949-2.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-4_8-branch/gcc/config/rs6000/rs6000.opt
    branches/gcc-4_8-branch/gcc/doc/invoke.texi
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog