[X86][MS] Fix the wrong alignment of vector variable arguments on Win32

[pengfei]

Summary

D108887 fixed alignment mismatch by changing the caller's alignment in
ABI. However, we found some cases that still assume the alignment is
vector size. This patch fixes them to avoid the runtime crash.

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

3475

I would prefer to see if we can avoid changing the prototype here. The target checks (is win32) can be calculated internally by looking at the subtarget. The only thing that varies per call site is the IsVarArg part. If we have to change the prototype, please just pass IsVarArg.

4092

Ditto. In this case, we are accumulating consecutive boolean parameters which can reduce readability as well. An alternative solution would be nicer.

4101

Surely there are other kinds of parameters that are clamped to 4 byte alignment on win32. How are doubles handled? Can we handle them the same way?

llvm/test/CodeGen/X86/vaargs-win32.ll

41

As I understand it, clang will not generate this IR. It will either mark the vector with inreg, or it will pass it indirectly (<4 x float>*).

97

I would like to see a test case where we set up a call that has intentionally misaligned parameters, so a C prototype that looks like void f(v4f, int, v4f, ...). This really underscores the need to use movups, because the ABI requires the data to be unaligned.

[pengfei]

Address @rnk 's comments. Thanks for the review!

llvm/lib/Target/X86/X86ISelLowering.cpp

4101

I think double might be aligned to 8 too. But I think ignore double handling should be ok. There's no difference with alignment equals to 4 and 8, because we don't have aligned instructions for it.

llvm/test/CodeGen/X86/vaargs-win32.ll

41

Yeah, it did have inreg when generated. I removed it because I thought it's not precise here. Adding it back.

97

We have a similar one above, see testPastArguments. It checks (int, v4f). I think it should be ok.
OTOH, we can also check whether or not to use movups by checking the stack realignment code andl $-16, %esp.

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

4101

I think it's important to make sure we have the right alignment values for all types, regardless of whether they have aligned instructions or not. LLVM uses alignment aggressively, so we need to be precise everywhere.

I'd still like to see if we can make this condition less target-specific. What's special about win32 in this case is that we only have 4 byte stack alignment. There are other platforms where this is true as well: i686-darwin for example.

What is the effect of passing Subtarget.getFrameLowering()->getStackAlign() in place of the MaybeAlign parameter?Presumably it would cause some test failures, but maybe we actually want that behavior.

If we do this, do we actually need the IsVarArg parameter at all? To me, it seems unlikely that whether a prototype has varargs or not should affect the way that prototyped arguments are passed. I believe such vectors passed in memory should be passed indirectly.

[pengfei]

llvm/lib/Target/X86/X86ISelLowering.cpp

4101

Sorry for the late reply.

What's special about win32 in this case is that we only have 4 byte stack alignment.

That's true, but some variables have their own alignment. See the example in f2. https://godbolt.org/z/xqcvj4YoK
I have this impression since I met the problem previously, but I'm not sure for other types. Seems double is still aligned to 4 byte.

There are other platforms where this is true as well: i686-darwin for example.

Seems not. At least f80 is aligned to 16 bytes. I just fixed the issue in D113739.

do we actually need the IsVarArg parameter at all?

Yes, because the same vector has different alignment between variant and fixed argument.

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

4101

Yes, LLVM will realign the stack to store values with high required alignment. I'm saying that, this code, which stores an argument to stack memory, should maybe clamp its alignment assumption to the ABI's stack alignment, which on Windows, happens to be 4. On most other platforms, it will be 16. That seems equivalent to your logic, and more general. I'm asking if this suggestion causes problems in practice. Maybe it causes widespread test failures, I can't say for sure. In any case, I'd like to see a more principled solution.

Yes, because the same vector has different alignment between variant and fixed argument.

I think what I'm trying to get at is that, in these two prototypes, v should be passed identically:

void f1(int x, v4f v, int y);
void f2(int x, v4f v, int y, ...);

The IsVarArg boolean affects the entire call site. Adding and removing an ellipsis to the prototype should not change the instructions used to store the prototyped arguments, they should remain the same, and use the regular alignment assumptions, right?

[pengfei]

Sorry, I made a mistake when I wanted to demonstrate the difference between variant and fixed arguments. Yes, you are right. The alignment I showed in f2 is the store of variable instead of ABI's.
A summary from my latest investigation:

1. For fixed function arguments:
   • LLVM will pass the first 3 vector variables by register: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86CallingConv.td#L804
   • LLVM will pass the following vector variables by value with alignment = 4: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86CallingConv.td#L788
   • Clang FE will emit the address of value instead of the value itself. So we don't have chance to handle the alignment. https://godbolt.org/z/KvvY4hMda
   • This is matching what MSVC's doing.
2. For variant arguments:
   • MSVC allows 3 vector variables at max no matter whether the variables are in the left of comma or in ellipsis in the prototype: https://godbolt.org/z/sYbcheEjv
   • MSVC always use stack to pass vector variables. The alignment for the vector variables is 4.

I think what I'm trying to get at is that, in these two prototypes, v should be passed identically:

No, they are not. On fixed arguments function, vector variables are passed by registers or address. While on variant function, vector variables are limited to 3 and passed by stack.

I'm saying that, this code, which stores an argument to stack memory, should maybe clamp its alignment assumption to the ABI's stack alignment, which on Windows, happens to be 4. On most other platforms, it will be 16.

We have specified each of the type's alignment in the calling conversion when they are passed by stack: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86CallingConv.td#L841
You can find on other 32 bit platforms, the type alignments are almost 4 too. The only exception is f80. Anyway, we don't need to warry about the fixed arguments here.

The IsVarArg boolean affects the entire call site.

Yes, because all vector variables are passed on stack with alignment = 4. No matter they are in the left of comma or right. It is intended to affects the entire call site. .

Adding and removing an ellipsis to the prototype should not change the instructions used to store the prototyped arguments, they should remain the same, and use the regular alignment assumptions, right?

No. Adding and removing an ellipsis are totally different ways when passing arguments. We need to use IsVarArg to distinguish each other.

[rnk]

In D114536#3191746, @pengfei wrote:

Sorry, I made a mistake when I wanted to demonstrate the difference between variant and fixed arguments. Yes, you are right. The alignment I showed in f2 is the store of variable instead of ABI's.
A summary from my latest investigation:

1. For fixed function arguments:
   • LLVM will pass the first 3 vector variables by register: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86CallingConv.td#L804
   • LLVM will pass the following vector variables by value with alignment = 4: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86CallingConv.td#L788
   • Clang FE will emit the address of value instead of the value itself. So we don't have chance to handle the alignment. https://godbolt.org/z/KvvY4hMda
   • This is matching what MSVC's doing.
2. For variant arguments:
   • MSVC allows 3 vector variables at max no matter whether the variables are in the left of comma or in ellipsis in the prototype: https://godbolt.org/z/sYbcheEjv
   • MSVC always use stack to pass vector variables. The alignment for the vector variables is 4.

I think what I'm trying to get at is that, in these two prototypes, v should be passed identically:

No, they are not. On fixed arguments function, vector variables are passed by registers or address. While on variant function, vector variables are limited to 3 and passed by stack.

I'm saying that, this code, which stores an argument to stack memory, should maybe clamp its alignment assumption to the ABI's stack alignment, which on Windows, happens to be 4. On most other platforms, it will be 16.

We have specified each of the type's alignment in the calling conversion when they are passed by stack: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86CallingConv.td#L841
You can find on other 32 bit platforms, the type alignments are almost 4 too. The only exception is f80. Anyway, we don't need to warry about the fixed arguments here.

The IsVarArg boolean affects the entire call site.

Yes, because all vector variables are passed on stack with alignment = 4. No matter they are in the left of comma or right. It is intended to affects the entire call site. .

Adding and removing an ellipsis to the prototype should not change the instructions used to store the prototyped arguments, they should remain the same, and use the regular alignment assumptions, right?

No. Adding and removing an ellipsis are totally different ways when passing arguments. We need to use IsVarArg to distinguish each other.

Thanks for the info, sorry about the delayed response. I see that adding the ellipsis drastically changes the way vectors are passed, but I think that complexity lives in the frontend. If the function is vararg, the frontend (Clang or other) will pass the vector directly. If it has a fixed prototype, the vector should be passed by address after passing three vectors.

There are no cases when a vector passed on the stack is aligned to 16 bytes, it should always be four byte aligned. Therefore, I don't think we need the IsVarArg boolean, we can go ahead and clamp the alignment on these argument loads.

[pengfei]

Remove IsVarArg boolean.

There are no cases when a vector passed on the stack is aligned to 16 bytes, it should always be four byte aligned.

Unfortunately, there is. See changes in win32-spill-xmm.ll.

The good news is it doesn't look like a reasonable test. See https://godbolt.org/z/hdsPTsbPW
MSVC doesn't pass a 512 bits argument in that way while Clang FE passes it by pointer.
It seems D12337 isn't a valid patch. So we can ignore the change in win32-spill-xmm.ll.
We may need to consider to fix the imcompatible issue between Clang and MSVC, but it is not related to this patch anyway.

[rnk]

lgtm

Sorry for the delay, I was out sick, and this fell out of my inbox.

llvm/lib/Target/X86/X86ISelLowering.cpp

4100

I think this could be simplified to use getStackAlign, but I won't insist.

[pengfei]

In D114536#3291195, @rnk wrote:

lgtm

Sorry for the delay, I was out sick, and this fell out of my inbox.

Thanks for the review. It's all right. Take care!

llvm/lib/Target/X86/X86ISelLowering.cpp

4100

I don't think so. The alignment are inconstant with different types on different OSs. Take CC_X86_64_C for example, https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86CallingConv.td#L592-L603 f80/f128 are determined by layout and vector types are aligned to their size.

llvm/lib/Target/X86/X86ISelLowering.cpp

 X86TargetLowering::LowerMemArgument(SDValue Chain, CallingConv::ID CallConv,
                                     const SmallVectorImpl<ISD::InputArg> &Ins,
                                     const SDLoc &dl, SelectionDAG &DAG,
                                     const CCValAssign &VA,
                                     MachineFrameInfo &MFI, unsigned i) const {
   // Create the nodes corresponding to a load from this parameter slot.
   ISD::ArgFlagsTy Flags = Ins[i].Flags;
   bool AlwaysUseMutable = shouldGuaranteeTCO(
       CallConv, DAG.getTarget().Options.GuaranteedTailCallOpt);
   bool isImmutable = !AlwaysUseMutable && !Flags.isByVal();
   EVT ValVT;
   MVT PtrVT = getPointerTy(DAG.getDataLayout());
 
   // If value is passed by pointer we have address passed instead of the value
   // itself. No need to extend if the mask value and location share the same
   // absolute size.
   bool ExtendedInMem =
X86TargetLowering::LowerMemArgument(SDValue Chain, CallingConv::ID CallConv,
 
   // Set SExt or ZExt flag.
   if (VA.getLocInfo() == CCValAssign::ZExt) {
     MFI.setObjectZExt(FI, true);
   } else if (VA.getLocInfo() == CCValAssign::SExt) {
     MFI.setObjectSExt(FI, true);
   }
 
+  MaybeAlign Alignment;
+  if (Subtarget.isTargetWindowsMSVC() && !Subtarget.is64Bit() &&
+      ValVT != MVT::f80)
+    Alignment = MaybeAlign(4);
   SDValue FIN = DAG.getFrameIndex(FI, PtrVT);
   SDValue Val = DAG.getLoad(
       ValVT, dl, Chain, FIN,
-      MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI));
+      MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI),
+      Alignment);
   return ExtendedInMem
              ? (VA.getValVT().isVector()
                     ? DAG.getNode(ISD::SCALAR_TO_VECTOR, dl, VA.getValVT(), Val)
                     : DAG.getNode(ISD::TRUNCATE, dl, VA.getValVT(), Val))
              : Val;
 }
 
 // FIXME: Get this from tablegen.

 }
 
 SDValue X86TargetLowering::LowerMemOpCallTo(SDValue Chain, SDValue StackPtr,
                                             SDValue Arg, const SDLoc &dl,
                                             SelectionDAG &DAG,
                                             const CCValAssign &VA,
                                             ISD::ArgFlagsTy Flags,
                                             bool isByVal) const {
   unsigned LocMemOffset = VA.getLocMemOffset();
   SDValue PtrOff = DAG.getIntPtrConstant(LocMemOffset, dl);
   PtrOff = DAG.getNode(ISD::ADD, dl, getPointerTy(DAG.getDataLayout()),
                        StackPtr, PtrOff);
   if (isByVal)
     return CreateCopyOfByValArgument(Arg, PtrOff, Chain, Flags, DAG, dl);
 
+  MaybeAlign Alignment;
+  if (Subtarget.isTargetWindowsMSVC() && !Subtarget.is64Bit() &&
+      Arg.getSimpleValueType() != MVT::f80)
+    Alignment = MaybeAlign(4);
   return DAG.getStore(
       Chain, dl, Arg, PtrOff,
-      MachinePointerInfo::getStack(DAG.getMachineFunction(), LocMemOffset));
+      MachinePointerInfo::getStack(DAG.getMachineFunction(), LocMemOffset),
+      Alignment);
 }
 
 /// Emit a load of return address if tail call
 /// optimization is performed and it is required.
 SDValue X86TargetLowering::EmitTailCallLoadRetAddr(
     SelectionDAG &DAG, SDValue &OutRetAddr, SDValue Chain, bool IsTailCall,
     bool Is64Bit, int FPDiff, const SDLoc &dl) const {
   // Adjust the Return address stack slot.

llvm/test/CodeGen/X86/vaargs-win32.ll

 ; MINGW-NEXT:    popl %ebp
 ; MINGW-NEXT:    retl
 entry:
   %0 = load <4 x float>, <4 x float>* @a, align 16
   %call = tail call i32 (i32, ...) @testm128(i32 1, <4 x float> inreg %0)
   ret void
 }
 
-define <4 x i32> @foo(<4 x float> %0, ...) nounwind {
+define <4 x i32> @foo(<4 x float> inreg %0, ...) nounwind {
 ; MSVC-LABEL: foo:
 ; MSVC:       # %bb.0:
 ; MSVC-NEXT:    pushl %eax
-; MSVC-NEXT:    movaps 8(%esp), %xmm0
+; MSVC-NEXT:    movups 8(%esp), %xmm0
 ; MSVC-NEXT:    movups 24(%esp), %xmm1
 ; MSVC-NEXT:    cmpltps %xmm1, %xmm0
 ; MSVC-NEXT:    popl %eax
 ; MSVC-NEXT:    retl
 ;
 ; MINGW-LABEL: foo:
 ; MINGW:       # %bb.0:
 ; MINGW-NEXT:    pushl %ebp
; MINGW-NEXT:    retl
   ret <4 x i32> %7
 }
 
 define <4 x i32> @bar() nounwind {
 ; MSVC-LABEL: bar:
 ; MSVC:       # %bb.0:
 ; MSVC-NEXT:    subl $32, %esp
 ; MSVC-NEXT:    movaps {{.*#+}} xmm0 = [5.0E+0,6.0E+0,7.0E+0,8.0E+0]
-; MSVC-NEXT:    movaps %xmm0, 16(%esp)
+; MSVC-NEXT:    movups %xmm0, 16(%esp)
 ; MSVC-NEXT:    movaps {{.*#+}} xmm0 = [1.0E+0,2.0E+0,3.0E+0,4.0E+0]
-; MSVC-NEXT:    movaps %xmm0, (%esp)
+; MSVC-NEXT:    movups %xmm0, (%esp)
 ; MSVC-NEXT:    calll _foo
 ; MSVC-NEXT:    addl $32, %esp
 ; MSVC-NEXT:    retl
 ;
 ; MINGW-LABEL: bar:
 ; MINGW:       # %bb.0:
 ; MINGW-NEXT:    pushl %ebp
 ; MINGW-NEXT:    movl %esp, %ebp
 ; MINGW-NEXT:    andl $-16, %esp
 ; MINGW-NEXT:    subl $48, %esp
 ; MINGW-NEXT:    movaps {{.*#+}} xmm0 = [5.0E+0,6.0E+0,7.0E+0,8.0E+0]
 ; MINGW-NEXT:    movaps %xmm0, 16(%esp)
 ; MINGW-NEXT:    movaps {{.*#+}} xmm0 = [1.0E+0,2.0E+0,3.0E+0,4.0E+0]
 ; MINGW-NEXT:    movaps %xmm0, (%esp)
 ; MINGW-NEXT:    calll _foo
 ; MINGW-NEXT:    movl %ebp, %esp
 ; MINGW-NEXT:    popl %ebp
 ; MINGW-NEXT:    retl
   %1 = tail call <4 x i32> (<4 x float>, ...) @foo(<4 x float> <float 1.000000e+00, float 2.000000e+00, float 3.000000e+00, float 4.000000e+00>, <4 x float> <float 5.000000e+00, float 6.000000e+00, float 7.000000e+00, float 8.000000e+00>)
   ret <4 x i32> %1
 }
 
 declare i32 @testm128(i32, ...) nounwind
 declare void @llvm.va_start(i8*)
 declare void @llvm.lifetime.start.p0i8(i64, i8*)
 declare void @llvm.lifetime.end.p0i8(i64, i8*)

llvm/test/CodeGen/X86/win32-spill-xmm.ll

 ; RUN: llc -mcpu=generic -mtriple=i686-pc-windows-msvc -mattr=+sse < %s | FileCheck %s
 
 ; Check proper alignment of spilled vector
 
 ; CHECK-LABEL: spill_ok
 ; CHECK: subl    $32, %esp
-; CHECK: movaps  %xmm3, (%esp)
+; CHECK: movups  %xmm3, (%esp)
 ; CHECK: movl    $0, 16(%esp)
 ; CHECK: calll   _bar
 define void @spill_ok(i32, <16 x float> *) {
 entry:
   %2 = alloca i32, i32 %0
   %3 = load <16 x float>, <16 x float> * %1, align 64
   tail call void @bar(<16 x float> %3, i32 0) nounwind
   ret void