[AArch64] Use correct calling convention for each vararg

[lephilousophe]

Summary

While checking is tail call optimization is possible, the calling
convention applied to fixed arguments is not the correct one.
This implies for DarwinPCS that all arguments of a vararg function will
go to the stack although fixed ones can go in registers.

This prevents non-virtual thunks to be tail optimized although they are marked
as musttail.

[lephilousophe]

I forgot to say that it's a fix for https://github.com/llvm/llvm-project/issues/54025

[mstorsjo]

No objection wrt the windows code (as the tests still are passing), but I'd rather have someone more familiar with this code give an authoritative review.

[lephilousophe]

Thank you for the rerouting.

While being at it, I had to duplicate the code twice in the same function and I wonder if:

• it could be reworked to avoid this
• the first test is really useful, it seems contradictory with the second one (if all arguments are in registers, why the stack space should matter?)

[rnk]

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:

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.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

5988

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.

5997

Seems like another musttail bug for similar reasons.

6015

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.

6069

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.

6087

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?

[rnk]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

5988

So far I have failed to convince clang to use the byval attribute when generating code for AArch64, so maybe this is irrelevant.

[lephilousophe]

This patch isn't making any codegen changes here, so clearly the backend can already handle this case.

In fact I just copied the piece of code from the AArch64TargetLowering::LowerCall function.
By doing this I believe I just make things consistent between whether the TCO could take place and when TCO is done.

Now a noob question related to your comments.
I read that there is now a clang attribute to mark calls as musttail. Isn't these checks necessary for the case where user can ask for a tail call when it's not possible?

[rnk]

In D120622#3349584, @lephilousophe wrote:

Now a noob question related to your comments.

It's a reasonable question. :)

I read that there is now a clang attribute to mark calls as musttail. Isn't these checks necessary for the case where user can ask for a tail call when it's not possible?

The frontend has a set of safety checks for the musttail attribute. If those pass, the IR verifier checks should also pass, meaning the IR prototypes should match. If the IR prototypes match, the backend should be able to handle it. If the fatal error I quoted earlier ever executes, that's a bug. I think we can fix more bugs than we introduce by removing that fatal error. There may be some constructs that AArch64 tail call codegen can't handle yet, but we need to fix them anyway.

[lephilousophe]

Is there something I could do to address all your comments?
Should I proceed, add the musttail check in LowerCall and remove the superfluous checks in IsEligibleForTailCallOptimization?

Is there other reviewers to add?

[rnk]

Ideally, I'd like to hear from @t.p.northover , since I think he added AArch64 musttail support, if I'm not mistaken. Maybe @efriedma can give input, I think we've discussed this feature and it's limitations before.

[efriedma]

In general, if we need to skip certain checks for musttail calls, I'd prefer to do that explicitly, check-by-check. Even for cases that "should" work, they require extra implementation work in some cases.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

5988

This is a missing feature. And yes, I don't think clang uses byval on AArch64 in practice.

5997

In general, we can't musttail in this case: x0 wouldn't be set correctly. From a C++ source code perspective, this should work because the caller and callee should use the same sret pointer value, but the IR verifier won't check that? Maybe it should?

For a practical example where this shows up, try the following targeting aarch64-pc-windows-msvc:

struct A { A(const A&); };
A f();
A g() { [[clang::musttail]] return f(); }

6015

We might be able to turn it into an indirect call...? Not sure.

6027–6028

Can you refactor this loop into a separate helper function, instead of copy-pasting it?

I think we need additional handling for musttail if there are more fixed arguments than fit in registers, but I guess we can leave that for a followup.

6069

For a musttail call, we should be able to reuse stack from the caller, but I think we'd need to explicitly implement that: we'd need to mess with the code that would normally allocate in the current stack frame.

6087

This naturally shouldn't fail for musttail calls, I think.

llvm/test/CodeGen/AArch64/darwinpcs-musttail.ll

17 ↗

(On Diff #411639)

Can you add a plain "tail" testcase in addition to the "musttail" testcase, since this patch affects the behavior of that too?

[rnk]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

5988

I came across this thread in my search, suggesting that byval wasn't implemented for aarch64 globalisel:
https://lists.llvm.org/pipermail/llvm-dev/2021-March/149031.html

5997

The same issue exists for x86 in RAX, right? I don't think we can write a verifier check for it, since it would make it invalid to obscure the sret pointer by spilling it to memory and reloading it. Obfuscators do this.

It seems like an existing issue that we should document as undefined behavior. If the sret pointer parameter isn't forwarded to a musttail call site, that's UB. It's more of a frontend concern than a user concern.

6015

On reflection, I think this is OK: if the user marks a direct call to an extern_weak function musttail, it's more reasonable to get a linker error or a runtime crash if that function is undefined during the link. There's just no way to honor the musttail marker and get the no-op behavior from the linker.

6027–6028

I think it's likely that no additional support is required for musttail. If we support passing memory arguments to other guaranteed TCO conventions like swifttailcc and tailcc, musttail should follow the same codepaths which overwrite the incoming memory arguments with the outgoing memory arguments before executing the epilogue. Having extra variadic arguments shouldn't matter, because when the prototypes match, it should not require allocating additional argument stack memory.

6050

This is the same ArgLocs computation, I think we could calculate it earlier and save it and reuse it, right?

6069

So, this *is* an interesting check. Without this check, we presumably allocate local stack memory, and pass a pointer to it to the tail call, which effectively creates a use-after-return bug. That's a real blocker.

[efriedma]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

5997

Yes, it the same issue as "RAX" on x86. I'm okay with runtime UB if we don't think it's a rule the verifier can reasonably verify. We still need to document the relevant rule in LangRef.

6027–6028

Oh, I see what you mean. Yes, musttail should just work if we skip this check.

Actually, taking another look, I'm not sure why we're special-casing varargs calls here in the first place; the CCInfo.getNextStackOffset() > FuncInfo->getBytesInStackArgArea() check later should catch the problematic cases, I think. Maybe I'm forgetting something.

[rnk]

I kind of side tracked the review, sorry about that. What's the next step here? Is it worth taking the time to rewrite this logic to refactor it and divide the musttail-relevant checks from the regular checks, or should we go forward with improving this patch?

I think I don't like the complexity in this patch. Ideally, I would like to declare non-musttail vararg tail calls to be unsupported: we shouldn't have to forward variadic arguments between functions with mismatched prototypes. To me, that's what's adding a lot of complexity in the eligibility checks here. One of the major use cases for the musttail feature is to make it *possible * to forward variadic argument packs because we don't have to worry about moving around arguments that live in memory.

[efriedma]

In D120622#3355792, @rnk wrote:

I kind of side tracked the review, sorry about that. What's the next step here? Is it worth taking the time to rewrite this logic to refactor it and divide the musttail-relevant checks from the regular checks, or should we go forward with improving this patch?

This patch looks more complicated than it actually is because it's copy-pasting code. The new code is doing exactly the same thing as the old code, except it's using the target-specific form of AnalyzeCallOperands.

Like I said before, I'd rather keep the codepaths unified if possible. If we need to skip some specific check for musttail, we can do that explicitly. There are multiple cases where musttail actually doesn't work, and a compiler crash in those cases seems better than silently miscompiling code.

I think I don't like the complexity in this patch. Ideally, I would like to declare non-musttail vararg tail calls to be unsupported: we shouldn't have to forward variadic arguments between functions with mismatched prototypes.

Forwarding variadic arguments isn't a thing outside of musttail. For non-musttail calls, whether the call is variadic doesn't really change the necessary checks.

[lephilousophe]

I factorized the code to have a unique function in charge to calculate the arguments locations.

[lephilousophe]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

6027–6028

I refactored the code in a separate function and merged in it varargs and non varargs case.
I don't think it would change the behaviour but it seems more correct to me that the checks in isEligibleForTailCallOptimization and in LowerCall are the same.

6050

I did it and tests pass.
By doing this, I relocated both checks at the same place it seemed to me it wasn't changing anything.

llvm/test/CodeGen/AArch64/darwinpcs-musttail.ll

17 ↗

(On Diff #411639)

I don't see how ot do this test as tail calls don't support ellipses at the end to call back varargs.

[lephilousophe]

Update to please clang-format

[efriedma]

llvm/test/CodeGen/AArch64/darwinpcs-musttail.ll

17 ↗

(On Diff #411639)

tail call void (%class.C*, i8*, ...) @_ZN1C3addEPKcz(%class.C* noundef nonnull align 8 dereferenceable(28) undef, i8* noundef %1) should trigger the relevant codepath, I think?

[lephilousophe]

Added a tail test case

[lephilousophe]

llvm/test/CodeGen/AArch64/darwinpcs-musttail.ll

17 ↗

(On Diff #411639)

Indeed. Thanks!
I renamed the file name in the same time.

[lephilousophe]

clang-format fixes again

[lephilousophe]

@efriedma is there something else to do to get this merged?
Thanks.

[efriedma]

LGTM. Sorry about the delay.

@lephilousophe I assume you want me to commit this for you; how should I credit you in the "author" line of the git commit?

@rnk any last comments before I merged?

[lephilousophe]

In D120622#3368010, @efriedma wrote:

LGTM. Sorry about the delay.

@lephilousophe I assume you want me to commit this for you; how should I credit you in the "author" line of the git commit?

You can use Philippe Valembois <[e-mail]>.

[rnk]

Thanks, this looks pretty good, but of course I dug into some edge cases.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

5908

Does this need to be a class method? Can it be a static helper with a TLI parameter?

5909–5910

CLI contains isVararg, CalleeCC, and Outs, I believe

6055

We should skip this check for musttaill calls (CLI.CB && CLI.CB->isMustTailCall())

6064

I think we'll still come here and have the same bug for variadic functions with 9 parameters when one of the fixed params is passed in memory, consider:
https://gcc.godbolt.org/z/Pb5EoK1d1

struct A { virtual void f(int x0, int x1, int x2, int x3, int x4, int x5, int x6, int x7, int mem, ...); };
struct B { virtual void f(int x0, int x1, int x2, int x3, int x4, int x5, int x6, int x7, int mem, ...); };
struct C : A, B {
    virtual void key();
    virtual void f(int x0, int x1, int x2, int x3, int x4, int x5, int x6, int x7, int mem, ...);
};

6183–6185

If you pass CLI here, the remaining parameters are redundant. All the parameters Outs, OutVals, Ins, CallConv, IsVarArg are members of CLI.

[lephilousophe]

Update to have a static function.
A test is added so musttail can be used even when fixed arguments are in stack.

[lephilousophe]

@rnk I think I addressed all your comments.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

5909–5910

Indeed.
This is now using CLI.
Subtarget is private in TLI so I pass it as an argument.

6055

Done.

6064

I added your test case to the regression tests.

6183–6185

Indeed. This is fixed.
Except that CallConnv was modified in the function and CLI didn't get updated.
I used a reference instead and CLI gets updated.
Tests are passing so I expect this change doesn't break anything.

[rnk]

lgtm, thanks!

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

6056

I strongly suspect it would be acceptable to simplify this to if (!Outs.empty()) return false;, but I don't want to insist.

llvm/test/CodeGen/AArch64/darwinpcs-tail.ll

35

Thanks for testing it.

llvm/lib/Target/AArch64/AArch64ISelLowering.h

private:
   SDValue LowerMGATHER(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerMSCATTER(SDValue Op, SelectionDAG &DAG) const;
 
   SDValue LowerMLOAD(SDValue Op, SelectionDAG &DAG) const;
 
   SDValue LowerINTRINSIC_W_CHAIN(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const;
 
+  void analyzeCallOperands(const CallLoweringInfo &CLI,
+      CallingConv::ID CalleeCC, bool isVarArg,
+      const SmallVectorImpl<ISD::OutputArg> &Outs,
+      CCState &CCInfo) const;
+
   bool isEligibleForTailCallOptimization(
       SDValue Callee, CallingConv::ID CalleeCC, bool isVarArg,
       const SmallVectorImpl<ISD::OutputArg> &Outs,
       const SmallVectorImpl<SDValue> &OutVals,
-      const SmallVectorImpl<ISD::InputArg> &Ins, SelectionDAG &DAG) const;
+      const SmallVectorImpl<ISD::InputArg> &Ins,
+      const CallLoweringInfo &CLI) const;
 
   /// Finds the incoming stack arguments which overlap the given fixed stack
   /// object and incorporates their load into the current chain. This prevents
   /// an upcoming store from clobbering the stack argument before it's used.
   SDValue addTokenForArgument(SDValue Chain, SelectionDAG &DAG,
                               MachineFrameInfo &MFI, int ClobberedFI) const;
 
   bool DoesCalleeRestoreStack(CallingConv::ID CallCC, bool TailCallOpt) const;

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

static bool mayTailCallThisCC(CallingConv::ID CC) {
   case CallingConv::Tail:
   case CallingConv::Fast:
     return true;
   default:
     return false;
   }
 }
 
+void AArch64TargetLowering::analyzeCallOperands(
+    const CallLoweringInfo &CLI,
+    CallingConv::ID CalleeCC, bool isVarArg,
+    const SmallVectorImpl<ISD::OutputArg> &Outs,
+    CCState &CCInfo) const {
+
+  SelectionDAG &DAG = CLI.DAG;
+  bool IsCalleeWin64 = Subtarget->isCallingConvWin64(CalleeCC);
+
+  unsigned NumArgs = Outs.size();
+  for (unsigned i = 0; i != NumArgs; ++i) {
+    MVT ArgVT = Outs[i].VT;
+    ISD::ArgFlagsTy ArgFlags = Outs[i].Flags;
+
+    bool UseVarArgCC = false;
+    if (isVarArg) {
+      // On Windows, the fixed arguments in a vararg call are passed in GPRs
+      // too, so use the vararg CC to force them to integer registers.
+      if (IsCalleeWin64) {
+        UseVarArgCC = true;
+      } else {
+        UseVarArgCC = !Outs[i].IsFixed;
+      }
+    } else {
+      // Get type of the original argument.
+      EVT ActualVT = getValueType(DAG.getDataLayout(),
+                                  CLI.Args[Outs[i].OrigArgIndex].Ty,
+                                  /*AllowUnknown*/ true);
+      MVT ActualMVT = ActualVT.isSimple() ? ActualVT.getSimpleVT() : ArgVT;
+      // If ActualMVT is i1/i8/i16, we should set LocVT to i8/i8/i16.
+      if (ActualMVT == MVT::i1 || ActualMVT == MVT::i8)
+        ArgVT = MVT::i8;
+      else if (ActualMVT == MVT::i16)
+        ArgVT = MVT::i16;
+    }
+
+    CCAssignFn *AssignFn = CCAssignFnForCall(CalleeCC, UseVarArgCC);
+    bool Res = AssignFn(i, ArgVT, ArgVT, CCValAssign::Full, ArgFlags, CCInfo);
+    assert(!Res && "Call operand has unhandled type");
+    (void)Res;
+  }
+}
+
 bool AArch64TargetLowering::isEligibleForTailCallOptimization(
     SDValue Callee, CallingConv::ID CalleeCC, bool isVarArg,
     const SmallVectorImpl<ISD::OutputArg> &Outs,
     const SmallVectorImpl<SDValue> &OutVals,
-    const SmallVectorImpl<ISD::InputArg> &Ins, SelectionDAG &DAG) const {
+    const SmallVectorImpl<ISD::InputArg> &Ins, const CallLoweringInfo &CLI) const {
   if (!mayTailCallThisCC(CalleeCC))
     return false;
 
+  const SelectionDAG &DAG = CLI.DAG;
   MachineFunction &MF = DAG.getMachineFunction();
   const Function &CallerF = MF.getFunction();
   CallingConv::ID CallerCC = CallerF.getCallingConv();
 
   // Functions using the C or Fast calling convention that have an SVE signature
   // preserve more registers and should assume the SVE_VectorCall CC.
   // The check for matching callee-saved regs will determine whether it is
   // eligible for TCO.
bool AArch64TargetLowering::isEligibleForTailCallOptimization(
 
   // Byval parameters hand the function a pointer directly into the stack area
   // we want to reuse during a tail call. Working around this *is* possible (see
   // X86) but less efficient and uglier in LowerCall.
   for (Function::const_arg_iterator i = CallerF.arg_begin(),
                                     e = CallerF.arg_end();
        i != e; ++i) {
     if (i->hasByValAttr())
       return false;
 
     // On Windows, "inreg" attributes signify non-aggregate indirect returns.
     // In this case, it is necessary to save/restore X0 in the callee. Tail
     // call opt interferes with this. So we disable tail call opt when the
     // caller has an argument with "inreg" attribute.
 
     // FIXME: Check whether the callee also has an "inreg" argument.
     if (i->hasInRegAttr())
       return false;
   }
 
   if (canGuaranteeTCO(CalleeCC, getTargetMachine().Options.GuaranteedTailCallOpt))
     return CCMatch;
 
   // Externally-defined functions with weak linkage should not be
   // tail-called on AArch64 when the OS does not support dynamic
   // pre-emption of symbols, as the AAELF spec requires normal calls
   // to undefined weak functions to be replaced with a NOP or jump to the
   // next instruction. The behaviour of branch instructions in this
   // situation (as used for tail calls) is implementation-defined, so we
   // cannot rely on the linker replacing the tail call with a return.
   if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
     const GlobalValue *GV = G->getGlobal();
     const Triple &TT = getTargetMachine().getTargetTriple();
     if (GV->hasExternalWeakLinkage() &&
         (!TT.isOSWindows() || TT.isOSBinFormatELF() || TT.isOSBinFormatMachO()))
       return false;
   }
 
   // Now we search for cases where we can use a tail call without changing the
   // ABI. Sibcall is used in some places (particularly gcc) to refer to this
   // concept.
 
   // I want anyone implementing a new calling convention to think long and hard
   // about this assert.
   assert((!isVarArg || CalleeCC == CallingConv::C) &&
          "Unexpected variadic calling convention");
 
   LLVMContext &C = *DAG.getContext();
-  if (isVarArg && !Outs.empty()) {
-    // At least two cases here: if caller is fastcc then we can't have any
-    // memory arguments (we'd be expected to clean up the stack afterwards). If
-    // caller is C then we could potentially use its argument area.
-
-    // FIXME: for now we take the most conservative of these in both cases:
-    // disallow all variadic memory operands.
-    SmallVector<CCValAssign, 16> ArgLocs;
-    CCState CCInfo(CalleeCC, isVarArg, MF, ArgLocs, C);
-
-    CCInfo.AnalyzeCallOperands(Outs, CCAssignFnForCall(CalleeCC, true));
-    for (const CCValAssign &ArgLoc : ArgLocs)
-      if (!ArgLoc.isRegLoc())
-        return false;
-  }
-
   // Check that the call results are passed in the same way.
   if (!CCState::resultsCompatible(CalleeCC, CallerCC, MF, C, Ins,
                                   CCAssignFnForCall(CalleeCC, isVarArg),
                                   CCAssignFnForCall(CallerCC, isVarArg)))
     return false;
   // The callee has to preserve all registers the caller needs to preserve.
   const AArch64RegisterInfo *TRI = Subtarget->getRegisterInfo();
   const uint32_t *CallerPreserved = TRI->getCallPreservedMask(MF, CallerCC);
   if (!CCMatch) {
     const uint32_t *CalleePreserved = TRI->getCallPreservedMask(MF, CalleeCC);
     if (Subtarget->hasCustomCallingConv()) {
       TRI->UpdateCustomCallPreservedMask(MF, &CallerPreserved);
       TRI->UpdateCustomCallPreservedMask(MF, &CalleePreserved);
     }
     if (!TRI->regmaskSubsetEqual(CallerPreserved, CalleePreserved))
       return false;
   }
 
   // Nothing more to check if the callee is taking no arguments
   if (Outs.empty())
     return true;
 
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CalleeCC, isVarArg, MF, ArgLocs, C);
 
-  CCInfo.AnalyzeCallOperands(Outs, CCAssignFnForCall(CalleeCC, isVarArg));
+  analyzeCallOperands(CLI, CalleeCC, isVarArg, Outs, CCInfo);
+
+  if (isVarArg) {
+    // At least two cases here: if caller is fastcc then we can't have any
+    // memory arguments (we'd be expected to clean up the stack afterwards). If
+    // caller is C then we could potentially use its argument area.
+
+    // FIXME: for now we take the most conservative of these in both cases:
+    // disallow all variadic memory operands.
+    for (const CCValAssign &ArgLoc : ArgLocs)
+      if (!ArgLoc.isRegLoc())
+        return false;
+  }
 
   const AArch64FunctionInfo *FuncInfo = MF.getInfo<AArch64FunctionInfo>();
 
   // If any of the arguments is passed indirectly, it must be SVE, so the
   // 'getBytesInStackArgArea' is not sufficient to determine whether we need to
   // allocate space on the stack. That is why we determine this explicitly here
   // the call cannot be a tailcall.
   if (llvm::any_of(ArgLocs, [](CCValAssign &A) {
         assert((A.getLocInfo() != CCValAssign::Indirect ||
                 A.getValVT().isScalableVector()) &&
                "Expected value to be scalable");
         return A.getLocInfo() == CCValAssign::Indirect;
       }))
     return false;
 
   // If the stack arguments for this call do not fit into our own save area then
   // the call cannot be made tail.
   if (CCInfo.getNextStackOffset() > FuncInfo->getBytesInStackArgArea())
     return false;
 
   const MachineRegisterInfo &MRI = MF.getRegInfo();
   if (!parametersInCSRMatch(MRI, CallerPreserved, ArgLocs, OutVals))
     return false;
 
   return true;
 }
 
 SDValue AArch64TargetLowering::addTokenForArgument(SDValue Chain,
                                                    SelectionDAG &DAG,
                                                    MachineFrameInfo &MFI,
AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
 
   MachineFunction &MF = DAG.getMachineFunction();
   MachineFunction::CallSiteInfo CSInfo;
   bool IsThisReturn = false;
 
   AArch64FunctionInfo *FuncInfo = MF.getInfo<AArch64FunctionInfo>();
   bool TailCallOpt = MF.getTarget().Options.GuaranteedTailCallOpt;
   bool IsSibCall = false;
-  bool IsCalleeWin64 = Subtarget->isCallingConvWin64(CallConv);
 
   // Check callee args/returns for SVE registers and set calling convention
   // accordingly.
   if (CallConv == CallingConv::C || CallConv == CallingConv::Fast) {
     bool CalleeOutSVE = any_of(Outs, [](ISD::OutputArg &Out){
       return Out.VT.isScalableVector();
     });
     bool CalleeInSVE = any_of(Ins, [](ISD::InputArg &In){
       return In.VT.isScalableVector();
     });
 
     if (CalleeInSVE || CalleeOutSVE)
       CallConv = CallingConv::AArch64_SVE_VectorCall;
   }
 
   if (IsTailCall) {
     // Check if it's really possible to do a tail call.
-    IsTailCall = isEligibleForTailCallOptimization(
-        Callee, CallConv, IsVarArg, Outs, OutVals, Ins, DAG);
+    IsTailCall = isEligibleForTailCallOptimization(Callee, CallConv, IsVarArg,
+                                                   Outs, OutVals, Ins, CLI);
 
     // A sibling call is one where we're under the usual C ABI and not planning
     // to change that but can still do a tail call:
     if (!TailCallOpt && IsTailCall && CallConv != CallingConv::Tail &&
         CallConv != CallingConv::SwiftTail)
       IsSibCall = true;
 
     if (IsTailCall)
       ++NumTailCalls;
   }
 
   if (!IsTailCall && CLI.CB && CLI.CB->isMustTailCall())
     report_fatal_error("failed to perform tail call elimination on a call "
                        "site marked musttail");
 
   // Analyze operands of the call, assigning locations to each operand.
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
 
   if (IsVarArg) {
-    // Handle fixed and variable vector arguments differently.
-    // Variable vector arguments always go into memory.
     unsigned NumArgs = Outs.size();
 
     for (unsigned i = 0; i != NumArgs; ++i) {
-      MVT ArgVT = Outs[i].VT;
-      if (!Outs[i].IsFixed && ArgVT.isScalableVector())
+      if (!Outs[i].IsFixed && Outs[i].VT.isScalableVector())
         report_fatal_error("Passing SVE types to variadic functions is "
                            "currently not supported");
-
-      ISD::ArgFlagsTy ArgFlags = Outs[i].Flags;
-      bool UseVarArgCC = !Outs[i].IsFixed;
-      // On Windows, the fixed arguments in a vararg call are passed in GPRs
-      // too, so use the vararg CC to force them to integer registers.
-      if (IsCalleeWin64)
-        UseVarArgCC = true;
-      CCAssignFn *AssignFn = CCAssignFnForCall(CallConv, UseVarArgCC);
-      bool Res = AssignFn(i, ArgVT, ArgVT, CCValAssign::Full, ArgFlags, CCInfo);
-      assert(!Res && "Call operand has unhandled type");
-      (void)Res;
-    }
-  } else {
-    // At this point, Outs[].VT may already be promoted to i32. To correctly
-    // handle passing i8 as i8 instead of i32 on stack, we pass in both i32 and
-    // i8 to CC_AArch64_AAPCS with i32 being ValVT and i8 being LocVT.
-    // Since AnalyzeCallOperands uses Ins[].VT for both ValVT and LocVT, here
-    // we use a special version of AnalyzeCallOperands to pass in ValVT and
-    // LocVT.
-    unsigned NumArgs = Outs.size();
-    for (unsigned i = 0; i != NumArgs; ++i) {
-      MVT ValVT = Outs[i].VT;
-      // Get type of the original argument.
-      EVT ActualVT = getValueType(DAG.getDataLayout(),
-                                  CLI.getArgs()[Outs[i].OrigArgIndex].Ty,
-                                  /*AllowUnknown*/ true);
-      MVT ActualMVT = ActualVT.isSimple() ? ActualVT.getSimpleVT() : ValVT;
-      ISD::ArgFlagsTy ArgFlags = Outs[i].Flags;
-      // If ActualMVT is i1/i8/i16, we should set LocVT to i8/i8/i16.
-      if (ActualMVT == MVT::i1 || ActualMVT == MVT::i8)
-        ValVT = MVT::i8;
-      else if (ActualMVT == MVT::i16)
-        ValVT = MVT::i16;
-
-      CCAssignFn *AssignFn = CCAssignFnForCall(CallConv, /*IsVarArg=*/false);
-      bool Res = AssignFn(i, ValVT, ValVT, CCValAssign::Full, ArgFlags, CCInfo);
-      assert(!Res && "Call operand has unhandled type");
-      (void)Res;
     }
   }
 
+  analyzeCallOperands(CLI, CallConv, IsVarArg, Outs, CCInfo);
+
   // Get a count of how many bytes are to be pushed on the stack.
   unsigned NumBytes = CCInfo.getNextStackOffset();
 
   if (IsSibCall) {
     // Since we're not changing the ABI to make this a tail call, the memory
     // operands are already available in the caller's incoming argument space.
     NumBytes = 0;
   }

llvm/test/CodeGen/AArch64/darwinpcs-tail.ll

+; With Darwin PCS, non-virtual thunks generated are generated with musttail
+; and are expected to build
+; In general Darwin PCS should be tail optimized
+; RUN: llc -mtriple=arm64-apple-ios5.0.0 < %s | FileCheck %s
+
+; CHECK-LABEL: __ZThn16_N1C3addEPKcz:
+; CHECK:       b __ZN1C3addEPKcz
+; CHECK-LABEL: _tailTest:
+; CHECK:       b __ZN1C3addEPKcz
+
+%class.C = type { %class.A.base, [4 x i8], %class.B.base, [4 x i8] }
+%class.A.base = type <{ i32 (...)**, i32 }>
+%class.B.base = type <{ i32 (...)**, i32 }>
+
+declare void @_ZN1C3addEPKcz(%class.C*, i8*, ...) unnamed_addr #0 align 2
+
+define void @_ZThn16_N1C3addEPKcz(%class.C* %0, i8* %1, ...) unnamed_addr #0 align 2 {
+  musttail call void (%class.C*, i8*, ...) @_ZN1C3addEPKcz(%class.C* noundef nonnull align 8 dereferenceable(28) undef, i8* noundef %1, ...)
+  ret void
+}
+
+define void @tailTest(%class.C* %0, i8* %1, ...) unnamed_addr #0 align 2 {
+  tail call void (%class.C*, i8*, ...) @_ZN1C3addEPKcz(%class.C* noundef nonnull align 8 dereferenceable(28) undef, i8* noundef %1)
+  ret void
+}