[PATCH] D120622: [AArch64] Use correct calling convention for each vararg

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I think AArch64 maintainers ought to reconsider the need to run TCO eligibility checks when musttail is set for the call. We have this code in the X86 backend before checking eligibility (file is too large for github to link to and show):

  if (isTailCall && !IsMustTail) {
    // Check if it's really possible to do a tail call.
    isTailCall = IsEligibleForTailCallOptimization(

In AArch64, we have this code <https://github.com/llvm/llvm-project/blob/f46890711f03dfb02722ec18dc332753967700e8/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L6157>:

  if (!IsTailCall && CLI.CB && CLI.CB->isMustTailCall())
    report_fatal_error("failed to perform tail call elimination on a call "
                       "site marked musttail");

Basically, on x86, musttail bypasses the majority of the checking. The verifier requirements for musttail are pretty strict: the prototypes must match, including ABI attributes, which is usually enough for the backend to actually implement TCO. This patch isn't making any codegen changes here, so clearly the backend can already handle this case.

The alternative is that we have to add a continuing number of special cases like the ones being added in this code.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5945
     if (i->hasByValAttr())
       return false;
 
----------------
This seems like a bug for musttail. We could set up a virtual thunk with a byval argument, and I believe that would result in the relevant backend error.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5954
     if (i->hasInRegAttr())
       return false;
   }
----------------
Seems like another musttail bug for similar reasons.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5972
         (!TT.isOSWindows() || TT.isOSBinFormatELF() || TT.isOSBinFormatMachO()))
       return false;
   }
----------------
This is an interesting check. If the user sets up a musttail call to an extern weak function, it's not clear what should happen. A codegen error seems reasonable.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6056
 
   // If any of the arguments is passed indirectly, it must be SVE, so the
   // 'getBytesInStackArgArea' is not sufficient to determine whether we need to
----------------
Suppose we had a musttail call passing such SVE arguments. Do we still need this legality check? If the prototypes match, won't the size of the allocated bytes be the same? For a given program, all SVE values have the same size at runtime, right? (forgive my ignorance)

Assuming yes, this represents another check that we would want to bypass for musttail calls. You can create a vtable thunk function for a method that passes SVE values to try to exercise this corner case.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6074
   const MachineRegisterInfo &MRI = MF.getRegInfo();
   if (!parametersInCSRMatch(MRI, CallerPreserved, ArgLocs, OutVals))
     return false;
----------------
Similarly, the ABI attribute checks in the verifier for musttail calls should imply that this check will always pass, we will never fail to TCO a musttail call for this reason, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120622