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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114536