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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 2 inline comments as done.
t.p.northover added a comment.

Thanks for the comments, I'll upload a new patch.



================
Comment at: llvm/docs/CodeGenerator.rst:2071
 * Caller and callee have the calling convention ``fastcc``, ``cc 10`` (GHC
-  calling convention), ``cc 11`` (HiPE calling convention), or ``tailcc``.
+  calling convention), ``cc 11`` (HiPE calling convention), ``tailcc``, or
+  ``swifttailcc``.
----------------
paquette wrote:
> paquette wrote:
> > Is `tailcc` supported on AArch64 yet? If not, this is somewhat misleading.
> oh, duh, this patch adds that
Yes. I debated whether it was closely related enough to include, but it just seemed silly to be updating exactly the places that would need to know about `tailcc` for it to work and not do it at the same time.


================
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
----------------
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?


================
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");
----------------
jroelofs wrote:
> @compnerd might know?
Possibly, but that's a whole other topic. Just making sure we fail noisily.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1751
+    // may clobber), convert it to a post-index ldp.
+    if (OffsetOp.getImm() == 0 && AfterCSRPopSize >= 0)
       convertCalleeSaveRestoreToSPPrePostIncDec(
----------------
jroelofs wrote:
> What if the pop size is larger than the immediate field in the ldp is able to encode?
The function is probably better named `tryConvert...`, if it can't fold it'll insert a normal  `sub sp, sp, #imm`.

But you're right I think there is a risk of overflow with the arg area possibly being large, and there's not actually an overflow check in that function. I'll add one.


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


================
Comment at: llvm/test/CodeGen/ARM/swifttailcc.ll:3
+
+define float @verify_aapcs_vfp(float %in) {
+; CHECK: vadd.f32 s0, s0, s0
----------------
jroelofs wrote:
> Is the swifttail attribute implicit here?
> 
> Seems like there should be tighter CHECKs for what is expected around the `vadd.f32`.
I have no idea what this file is here for. It should probably be part of the ARM implementation if anything, but even there it seems useless. I'll remove it from this review.


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

https://reviews.llvm.org/D95443