Page MenuHomePhabricator

[X86][MS] Fix the aligement mismatch of vector variable arguments on Win32
ClosedPublic

Authored by pengfei on Aug 29 2021, 12:53 AM.

Details

Summary

The alignment of vector variable arguments in callee side is 4, which is
aligned with MSVC. But the caller aligns them to the size of vector
arguments. It results in run fails. This patch fixes this problem by
trimming it to 4 bytes for variable arguments on Win32.

Fixed vector arguments are passed by pointer on Win32. So they don't have
the problem.

I don't find a doc in MSDN for this calling conversion, so I did several
experiments here: https://godbolt.org/z/n1zn1Gx1z

Diff Detail

Event Timeline

pengfei created this revision.Aug 29 2021, 12:53 AM
pengfei requested review of this revision.Aug 29 2021, 12:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2021, 12:53 AM
rnk added a comment.Aug 30 2021, 9:39 AM

Thanks!

llvm/lib/Target/X86/X86CallingConv.td
29–30

Please check what mingw does in this case.

pengfei updated this revision to Diff 369602.Aug 30 2021, 7:29 PM

Add test for mingw.

pengfei added inline comments.Aug 30 2021, 7:29 PM
llvm/lib/Target/X86/X86CallingConv.td
29–30

With this patch, mingw also uses 4 bytes alignment for both caller and callee.
But I'm not sure if it is the correct approach since I don't have a mingw environment to test.
@LiuChen3 fixed Linux 32 bits problem in D78564 by changing callee's alignment to the vector size. Not sure if we should fix in that way.

rnk added inline comments.Aug 30 2021, 8:32 PM
llvm/lib/Target/X86/X86CallingConv.td
29–30

@mstorsjo , do you mind checking what GCC does on Windows here? Mainly I'm looking out for the possibility that GCC passes such vectors indirectly when passed through varargs.

mstorsjo added inline comments.Sep 3 2021, 1:26 PM
llvm/lib/Target/X86/X86CallingConv.td
29–30

Sorry for the delay. I checked the testcase from Compiler Explorer with GCC targeting mingw (10.2), and I get this:

__Z8testm128iz:
        pushl   %ebp
        movl    %esp, %ebp
        leal    27(%ebp), %eax
        andl    $-16, %eax
        vmovups (%eax), %xmm0
        vmovups %xmm0, _res1
        movl    8(%ebp), %eax
        popl    %ebp
        ret
[...]
__Z17testPastArgumentsv:
        pushl   %ebp
        movl    %esp, %ebp
        andl    $-16, %esp
        subl    $32, %esp
        vmovups _a, %xmm0
        vmovups %xmm0, 16(%esp)
        movl    $1, (%esp)
        call    __Z8testm128iz
        leave
        ret

So based on that, I'd say that GCC consistently aligns them as a caller, and considers them aligned as a callee. So to be compatible, we'd need to do things the other way for mingw targets.

Then again, I guess such a thing could be considered a calling convention bug in GCC which might be fixable too (with a minor ABI break, but I guess such cases are rare in practice), and I'm not sure if they care about being calling convention compatible with MSVC regarding vector arguments either.

rnk added inline comments.Sep 3 2021, 1:50 PM
llvm/lib/Target/X86/X86CallingConv.td
29–30

No problem, I think this just means we should tweak this isOSWindows condition to isWindowsMSVCEnvironment.

How do I test mingw on compiler explorer? That would be handy for me.

mstorsjo added inline comments.Sep 3 2021, 2:02 PM
llvm/lib/Target/X86/X86CallingConv.td
29–30

I don't think compiler explorer has mingw GCC compilers unfortunately, so I just tested with a local cross compiler I happened to have around. (I have no idea why they don't provide that configuration there, especially as they have a MSVC-in-wine setup anyway.)

pengfei updated this revision to Diff 370688.Sep 3 2021, 6:47 PM

Keep the change working only for MSVC compatiblity. Mingw is using a different alignment.
Thanks Reid and Martin for the help.

pengfei updated this revision to Diff 370690.Sep 3 2021, 7:24 PM

Remove callee test since the alignment of va variables is generated in front end.

rnk accepted this revision.Sep 7 2021, 3:14 PM

lgtm, thanks!

This revision is now accepted and ready to land.Sep 7 2021, 3:14 PM