[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:
> > > > > mstorsjo wrote:
> > > > > > 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.
> > > > > Unfortunately, va_start translation for Windows has not been fully implemented in GlobalISel yet, that is the reason why I hadn't changed test files you mentioned.
> > > > > As far as I can test, I see no problems in these tests output with va_start call removed.
> > > > Ok, so this only handles some part of the variadic functions, but not everything of it? In that case I guess you won't hit this case quite yet, as that relates specifically to dumping the variadic arguments from registers, i.e. part of `va_start`.
> > > `aarch64_win64cc_vararg.ll` covers variable dumping, doesn't it? It triggers lines being discussed and seems to be ok without them.
> > > What else you think should be checked to hit the case? Sorry for misunderstanding.
> > I don't think there's anything else involved in that; the quirky code lines from the SelectionDAG implementation were specifically about variable dumping - so if that's not relevant here (yet), then you probably can leave out that bit entirely, for now.
> Thank you so much for the help!
It turns out that the missing case of `(GPRSaveSize & 15)` did indeed break something.

`aarch64_win64cc_vararg.ll` tests much fewer cases than `win64_vararg.ll`, and during this review, I was led to believe that this change wouldn't hit those cases.

I see that `win64_vararg.ll` fails to translate with GlobalISel (don't quite know why) so I see why you weren't able to test that easily.

However I believe the issue should have been possible to test by just testing various functions with both even and odd numbers of parameters. Unfortunately, the issue is quite nonobvious; without the fix, things do seem to work right, but the location of stack objects mismatch between AArch64FrameLowering and the stack objects that are listed in MIR - readding this removed line fixes that.

The issue caused by this is https://github.com/llvm/llvm-project/issues/64740. I've got a patch coming up, readding the lines that were left out here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130903