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.

Details

Summary

Reimplemented SelectionDAG code for GlobalISel.

Fixes https://github.com/llvm/llvm-project/issues/54079

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
553

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

564

nit: can you use static_cast for greppability?

565

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

568

nit: static_cast?

572

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

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

and use that everywhere?

574

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

I'm not sure I have clean understanding of this line. It was introduced here https://reviews.llvm.org/D35720

dzhidzhoev marked an inline comment as not done.Aug 2 2022, 4:42 PM
dzhidzhoev added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

clear*

dzhidzhoev marked an inline comment as not done.Aug 2 2022, 4:43 PM
mstorsjo added inline comments.Aug 2 2022, 11:52 PM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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.

llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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?

mstorsjo added inline comments.Aug 23 2022, 1:20 AM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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.

dzhidzhoev added inline comments.Aug 24 2022, 6:28 PM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

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
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
565

Thank you so much for the help!

arsenm added inline comments.Tue, Sep 20, 7:16 AM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
554

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