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

[Le Philousophe via Phabricator via llvm-commits]

lephilousophe added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5995
 
-    CCInfo.AnalyzeCallOperands(Outs, CCAssignFnForCall(CalleeCC, true));
+    unsigned NumArgs = Outs.size();
+    for (unsigned i = 0; i != NumArgs; ++i) {
----------------
efriedma wrote:
> rnk wrote:
> > efriedma wrote:
> > > 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.
> > 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.
> 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.
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6036
 
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CalleeCC, isVarArg, MF, ArgLocs, C);
----------------
rnk wrote:
> This is the same ArgLocs computation, I think we could calculate it earlier and save it and reuse it, right?
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.


================
Comment at: llvm/test/CodeGen/AArch64/darwinpcs-musttail.ll:17
+  ret void
+}
----------------
efriedma wrote:
> Can you add a plain "tail" testcase in addition to the "musttail" testcase, since this patch affects the behavior of that too?
I don't see how ot do this test as tail calls don't support ellipses at the end to call back varargs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120622