[Bug c/66275] New: __attribute__((sysv_abi)) with x86_64-w64-mingw32-gcc generates incorrect code

["peter at cordes dot ca" <gcc-bugzilla at gcc dot gnu dot org>]

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

            Bug ID: 66275
           Summary: __attribute__((sysv_abi)) with x86_64-w64-mingw32-gcc
                    generates incorrect code
           Product: gcc
           Version: 4.9.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: peter at cordes dot ca
  Target Milestone: ---

Created attachment 35615
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35615&action=edit
main.c, asm-test.h, and less-stripped-down intrin-pinsrw.c

gcc (Ubuntu 4.9.2-10ubuntu13) 4.9.2

w64 mingw gcc is mis-compiling functions declared with
__attribute__((sysv_abi)).  Registers holding args are getting clobbered with
temp values, but then used as if they still held function args.

This code is mis-compiled by
x86_64-w64-mingw32-gcc -g -Wall -funroll-loops -O3 -std=gnu11 intrin-pinsrw.c
... main.c


cat > intrin-pinsrw.c <<"EOF"
#include <emmintrin.h>
#include <stdint.h>

#define SYSV_ABI __attribute__((sysv_abi))


union wordbytes { uint16_t w; uint8_t b[2]; };

void SYSV_ABI rs_process_pinsrw_intrin(void* dstvoid, const void* srcvoid,
size_t size, const uint32_t* LH)
{
        const uint64_t *src = srcvoid;
        __m128i d, l, h;
        __m128i *dst = dstvoid;

        for (size_t i = 0; i < size/sizeof(d) ; i+=1) {
                uint64_t s0 = src[i*2 + 0];

#define LO(x) ((uint8_t)x)
#define HI(x) (( (union wordbytes) ((uint16_t)x) ).b[1])

                l = _mm_cvtsi32_si128( LH[      LO(s0)] );
                h = _mm_cvtsi32_si128( LH[256 + HI(s0)] );
                s0 >>= 16;
 // commented-out code that wasn't needed to trigger the bug
                d = _mm_xor_si128(l, h);
                d = _mm_xor_si128(d, dst[i]);
                _mm_store_si128(&dst[i], d);
        }
}
EOF
x86_64-w64-mingw32-gcc -Wall -funroll-loops -O3 -std=gnu11 intrin-pinsrw.c -S

search for "si" in the output, and notice how %rsi is used as a pointer to load
src data, but also used as a tmp.

.L5:
        movq    (%rsi,%r10), %rdx
        movzbl  %dl, %esi              ## clobbered here
        movzbl  %dh, %eax
        movd    (%rcx,%rsi,4), %xmm8
        movd    1024(%rcx,%rax,4), %xmm9
        pxor    %xmm8, %xmm9
        pxor    (%rdi,%r10), %xmm9
        movaps  %xmm9, (%rdi,%r10)
        movq    16(%rsi,%r10), %rdx    ## not restored after use as a tmp
        ...

%rdi is also clobbered and then used as a pointer again, later in the loop.

In the AMD64 sysv ABI, %rdi holds the 1st arg, %rsi holds the 2nd, %rdx holds
the 3rd, and %rcx holds the 4th.  

(and see attached for a less-stripped-down and a main.c to call it)

-funroll-loops is needed to trigger the problem on this reduced test-case.  It
might not be needed with a bigger function that would run out of registers
without unrolling.  Even the less-stripped-down version in the attachment runs
fine under WINE when compiled without -funroll-loops.


If testing by actually running the code with WINE, note that wine and wine64
seem to have incompatible requirements for the contents of ~/.wine.  I
eventually made a wrapper like:
 #!/bin/sh
 WINEPREFIX=~/.wine64 /usr/bin/wine64 "$@"


For anyone curious why I had this code in the first place:
This code is from par2 (the reed-solomon inner-loop that multiplies a buffer of
GF16 values by a constant factor, using a pre-computed lookup table).  See
https://github.com/pcordes/par2-asm-experiments

 I have some functions written directly in asm.  They're written for the Linux
SysV ABI.  I could make an alternate entry point for win ABI, with extra mov
instructions, and push/pop the extra regs that are callee-save in MS-ABI but
not SYSV.  Or I could just declare their prototypes with
__attribute__((sysv_abi)), and gcc will call them properly.

I tried making a version of the function using intrinsics, so of course my test
harness needs to call it, too.  Since I call via function pointer, I had to
declare it the same way.

It was working fine until a recent change to the code of the function.  Old
version that didn't tickle this bug (partial diff):

-       uint64_t s0, s1;
-       for (size_t i = 0; i < size ; i+=16) {
-               s0 = *((uint64_t*) ((char*)srcvoid + i));       // byte address
math, 64bit load
-               s1 = *((uint64_t*) ((char*)srcvoid+8 + i));     // byte address
math, 64bit load

-               d = _mm_xor_si128(d, *(dst+i/16));
-               _mm_store_si128(dst+i/16, d);

I wrote it in that ugly way initially because I was basically porting my ASM
code to intrinsics.  BTW, the results were terrible.  gcc generates
ridiculously bad code for getting the src bytes into zero-extended 64bit regs,
for use as scaled-offsets in an address, compared to

movzx %dl, %eax
movxz %dh, %ebx
shr   $16, %rdx
use rax/rbx
movzx %dl, %eax
movxz %dh, %ebx
shr   $16, %rdx
use rax/rbx
 ...

 gcc never just shifts the reg holding src data.  Instead if copies, and shifts
the copy by $16, $32, or $48.

 gcc's code is about 35% slower than the hand-written version, even letting it
use avx so it doesn't emit useless movdqa instructions when it doesn't realize
that an old value is no longer needed.  Just un-comment the mostly-commented
loop body in the testcase (attachment version).

 Anyway, slow code is off-topic, this bug is about wrong code!