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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:4095
+  if (IsVarArgWin32 && Arg.getSimpleValueType().isVector())
+    Alignment = MaybeAlign(4);
   return DAG.getStore(
----------------
rnk wrote:
> 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?
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.


================
Comment at: llvm/test/CodeGen/X86/vaargs-win32.ll:41
 ; MSVC-NEXT:    pushl %eax
-; MSVC-NEXT:    movaps 8(%esp), %xmm0
+; MSVC-NEXT:    movups 8(%esp), %xmm0
 ; MSVC-NEXT:    movups 24(%esp), %xmm1
----------------
rnk wrote:
> 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>*`).
Yeah, it did have `inreg` when generated. I removed it because I thought it's not precise here. Adding it back.


================
Comment at: llvm/test/CodeGen/X86/vaargs-win32.ll:97
 ; 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
----------------
rnk wrote:
> 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.
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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114536/new/

https://reviews.llvm.org/D114536