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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5954
     if (i->hasInRegAttr())
       return false;
   }
----------------
rnk wrote:
> efriedma wrote:
> > rnk wrote:
> > > Seems like another musttail bug for similar reasons.
> > 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(); }
> > ```
> 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.
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120622