[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:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114536