Page MenuHomePhabricator

[AArch64][GlobalISel] Lower formal arguments of AAPCS & ms_abi variadic functions.
Needs ReviewPublic

Authored by dzhidzhoev on Aug 1 2022, 7:35 AM.



Reimplemented SelectionDAG code for GlobalISel.


Diff Detail

Event Timeline

dzhidzhoev created this revision.Aug 1 2022, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 7:35 AM
dzhidzhoev requested review of this revision.Aug 1 2022, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 7:35 AM
dzhidzhoev updated this revision to Diff 449025.Aug 1 2022, 8:24 AM

Fix pre-merge checks fail.

paquette added inline comments.Aug 1 2022, 10:34 AM

It looks like these arrays already exist in AArch64CallingConvention.cpp; is it possible to use them here? (Or should we?)


nit: can you use static_cast for greppability?


Can you explain why this is the case in a comment? Even referencing some win64 doc would be useful?


nit: static_cast?


can you pull LLT::pointer(0, 64) out into like

const LLT p0 = LLT::pointer(0, 64);

and use that everywhere?


similar to above, can you create a

LLT s64 = LLT::scalar(64);

and use that everywhere?

dzhidzhoev updated this revision to Diff 449474.Aug 2 2022, 4:27 PM

Addressing @paquette comments.

dzhidzhoev added inline comments.Aug 2 2022, 4:29 PM

I'm not sure I have clean understanding of this line. It was introduced here

dzhidzhoev marked an inline comment as not done.Aug 2 2022, 4:42 PM
dzhidzhoev added inline comments.


dzhidzhoev marked an inline comment as not done.Aug 2 2022, 4:43 PM
mstorsjo added inline comments.Aug 2 2022, 11:52 PM

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.

Rebase & remove placeholder stack object.


It seems like these lines don't affect test output. Let's try to remove them.

mstorsjo added inline comments.Aug 20 2022, 12:52 PM

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.

dzhidzhoev added inline comments.Aug 22 2022, 4:34 PM

I've tested it on modified sample from Github issue .
It has shown the same output for GlobalISel and SelectionDAG.
Could you possibly suggest something else to check?

mstorsjo added inline comments.Aug 23 2022, 1:20 AM

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:

        sub     sp, sp, #16
        add     x8, sp, #8
        add     x0, sp, #8
        stp     x8, x7, [sp], #16

(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.

dzhidzhoev added inline comments.Aug 24 2022, 6:28 PM

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.

mstorsjo added inline comments.Aug 24 2022, 10:41 PM

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.

dzhidzhoev added inline comments.Aug 25 2022, 2:39 AM

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.

mstorsjo added inline comments.Aug 25 2022, 2:45 AM

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.

dzhidzhoev added inline comments.Aug 25 2022, 2:48 AM

Thank you so much for the help!

arsenm added inline comments.Tue, Sep 20, 7:16 AM

You should consistently get the calling convention from CCState, not the parent function. It's wrong in outgoing contexts