Reimplemented SelectionDAG code for GlobalISel.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
561 | It looks like these arrays already exist in AArch64CallingConvention.cpp; is it possible to use them here? (Or should we?) | |
572 | nit: can you use static_cast for greppability? | |
573 | Can you explain why this is the case in a comment? Even referencing some win64 doc would be useful? | |
576 | nit: static_cast? | |
580 | can you pull LLT::pointer(0, 64) out into like const LLT p0 = LLT::pointer(0, 64); and use that everywhere? | |
582 | similar to above, can you create a LLT s64 = LLT::scalar(64); and use that everywhere? |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | I'm not sure I have clean understanding of this line. It was introduced here https://reviews.llvm.org/D35720 |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | clear* |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | 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 | ||
---|---|---|
573 | It seems like these lines don't affect test output. Let's try to remove them. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | 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. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | I've tested it on modified sample from Github issue https://godbolt.org/z/ndbx96M9r . |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | 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. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | 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. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | 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. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | aarch64_win64cc_vararg.ll covers variable dumping, doesn't it? It triggers lines being discussed and seems to be ok without them. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | 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. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | Thank you so much for the help! |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
562 | You should consistently get the calling convention from CCState, not the parent function. It's wrong in outgoing contexts |
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp | ||
---|---|---|
1944 | Did you really mean the caller CC here, not the callee? |
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp | ||
---|---|---|
1944 | In these lines, we're lowering G_VASTART instruction. The branch is supposed to execute if the calling convention of MachineFunction containing G_VASTART is Win64. Do you think that this code does something else? |
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp | ||
---|---|---|
1944 | I know nothing about varargs, I just thought it was suspicious to see selection dependent on the parent calling convention |
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp | ||
---|---|---|
1944 | Got it. Thank you for paying attention. |
I had somewhat missed the scope of this patch - I had only expected it to affect how functions marked ms_abi on non-Windows platforms are lowered, while it affects all regular Windows targets too.
This caused a pretty severe regression for Windows targets, reproducible with this snippet:
$ cat test.c double func(double f) { return f; } $ clang -target aarch64-windows -S -o - test.c -fno-asynchronous-unwind-tables func: sub sp, sp, #16 str x0, [sp, #8] ldr d0, [sp, #8] add sp, sp, #16 ret
(The -fno-asynchronous-unwind-tables option is only relevant for clarity of the output.)
I'll go ahead and push a revert in a moment.
Thank you for catching it! I wonder how win64 tests didn't fail on that. The problem was that during rebase I accidentally changed CCAssignFn arguments, so that all Win64 function was treated as vararg. Fixed it now. Should I recommit?
Thanks for the update! I can run a bunch of tests on it that specifically exercise calling conventions (that I don't otherwise run regularly) - I'll let you know how it fares.
The tests I had ran just fine now, thanks!
However I don't see any added test for this case - could you add that too? (I guess it's probably covered by some existing test - just extending that to run with globalisel probably is enough?)
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
687 | Parameter name suggest this should just be isVarArg |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
687 | https://github.com/llvm/llvm-project/blob/9408164254d26d5305fe4e0267b668c41c1266ed/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp#L602 |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
687 | The way AArch64TargetLowering::CCAssignFnForCall works is a little weird. For Darwin calling conventions, "fixed" arguments to varargs functions are passed differently from "variadic" arguments. Instead of recording that information in the arguments themselves, we call a different AssignFn for fixed vs. variadic arguments. There are no such thing as variadic parameters, though; you can only access variadic arguments through va_arg. So we don't need to handle variadic arguments here. Maybe there's a better way to implement the whole thing (if we record whether an argument is fixed in ArgFlagsTy, we could unify the Darwin calling conventions), but this is consistent with the way it currently works for SelectionDAG ISel. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
687 | I vaguely remember encountering this mess before |
Addressed @mstorsjo comment. Added "foo" test function, causing
regression for Windows target on previous (buggy) patch version.
Thanks - I believe this should be ok to try relanding it then! I'll follow up again if it triggers something else somewhere, but it at least seems fine in initial testing now.
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | 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. |
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp | ||
---|---|---|
573 | Thank you for clarifying and fixing it! |
It looks like these arrays already exist in AArch64CallingConvention.cpp; is it possible to use them here? (Or should we?)