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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114536