[PATCH] D95443: IR/AArch64/X86: add "swifttailcc" calling convention.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:196
 
-/// Returns the argument pop size.
-static uint64_t getArgumentPopSize(MachineFunction &MF,
-                                   MachineBasicBlock &MBB) {
+// Returns how much of the incoming argument stack area we should clean up in an
+// epilogue. For the C calling convention this will be 0, for guaranteed tail
----------------
t.p.northover wrote:
> paquette wrote:
> > jroelofs wrote:
> > > measured in bytes?
> > Should be a doxygen comment?
> Done because it's harmless either way, but does doxygen care about static functions?
Yep: https://llvm.org/doxygen/AArch64InstructionSelector_8cpp.html


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:320
   } else {
+    assert(AFI->getTailCallReservedStack() == 0 &&
+           "don't know how guaranteed tail calls might work on Win64");
----------------
t.p.northover wrote:
> jroelofs wrote:
> > @compnerd might know?
> Possibly, but that's a whole other topic. Just making sure we fail noisily.
I feel like this should actually be something heavier than an assert? If someone builds without asserts and tries to use this on Windows, it'd be unfortunate if they ended up with something totally broken.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2383
         !RPI.isScalable() && RPI.Type != RegPairInfo::FPR128 &&
-        !RPI.isPaired()) {
+        !RPI.isPaired() && ByteOffset % 16 != 0) {
       ByteOffset += 8 * StackFillDir;
----------------
The assert in the original code checks that `ByteOffset % 16 == 0`, why did that change here?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:928
     MIB->getOperand(1).setImm(FPDiff);
-    CallSeqStart.addImm(NumBytes).addImm(0);
+    CallSeqStart.addImm(0).addImm(0);
     // End the call sequence *before* emitting the call. Normally, we would
----------------
t.p.northover wrote:
> paquette wrote:
> > Why did this change?
> Minor bugfix. The immediate tells stack allocation how much space it should reserve for this call's stack-based arguments, but because it's a tail call its stack space is actually allocated independently at the other end of the frame (before even the callee-saved registers get pushed).
> 
> The practical effect of this is that with `NumBytes` the function was allocating space in both places, using more stack than is necessary.
Ah, I see. Makes sense.


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

https://reviews.llvm.org/D95443