[PATCH] D130903: [AArch64][GlobalISel] Lower formal arguments of AAPCS & ms_abi variadic functions.

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:565
+      GPRIdx = MFI.CreateFixedObject(GPRSaveSize, -(int)GPRSaveSize, false);
+      if (GPRSaveSize & 15)
+        // The extra size here, if triggered, will always be 8.
----------------
dzhidzhoev wrote:
> mstorsjo wrote:
> > dzhidzhoev wrote:
> > > mstorsjo wrote:
> > > > dzhidzhoev wrote:
> > > > > dzhidzhoev wrote:
> > > > > > paquette wrote:
> > > > > > > Can you explain why this is the case in a comment? Even referencing some win64 doc would be useful?
> > > > > > I'm not sure I have clean understanding of this line. It was introduced here https://reviews.llvm.org/D35720
> > > > > clear*
> > > > The reason why it's always 8, is that if we're reserving space for a number of 8 byte registers and align it to 16, we either will have a 8 byte gap, or no gap at all.
> > > > 
> > > > As for the exact reasons why we manually added a gap in the SelectionDAG implementation, I don't really remember anything more than what you can read in the old phabricator review unfortunately.
> > > It seems like these lines don't affect test output. Let's try to remove them.
> > I can't guarantee that there aren't gaps in the testcases for this combination though, so it'd be good if you'd manually try to see if there are other cases that can be triggered (with either more or less formal parameters to the function, and/or more or less regular stack allocation.
> I've tested it on modified sample from Github issue https://godbolt.org/z/ndbx96M9r .
> It has shown the same output for GlobalISel and SelectionDAG.
> Could you possibly suggest something else to check? 
I don't know about that particular testcase or the GlobalISel implementation, but if referring to the existing code,
```
      if (GPRSaveSize & 15)
        // The extra size here, if triggered, will always be 8.
        MFI.CreateFixedObject(16 - (GPRSaveSize & 15), -(int)alignTo(GPRSaveSize, 16), false);
```
then if this is removed, three existing testcases under test/CodeGen/AArch64 fail/change output, `aarch64_win64cc_vararg.ll`, `sponentry.ll` and `win64_vararg.ll`. Have a look at what changes in their output. E.g. for the `win64_vararg.ll` testcase; there, the function `f7` ends up broken, looking like this:
```
f7:
        sub     sp, sp, #16
        add     x8, sp, #8
        add     x0, sp, #8
        stp     x8, x7, [sp], #16
        ret
```
(Here, both `x8` and `x7` are stored on the stack outside of the stack allocation.)

Please check at least all those cases that differ with the SelectionDAG implementation when this logic is removed, that they work correctly with GlobalISel too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130903