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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5945
     if (i->hasByValAttr())
       return false;
 
----------------
efriedma wrote:
> rnk wrote:
> > rnk wrote:
> > > 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.
> > So far I have failed to convince clang to use the `byval` attribute when generating code for AArch64, so maybe this is irrelevant.
> This is a missing feature.  And yes, I don't think clang uses byval on AArch64 in practice.
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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5954
     if (i->hasInRegAttr())
       return false;
   }
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5972
         (!TT.isOSWindows() || TT.isOSBinFormatELF() || TT.isOSBinFormatMachO()))
       return false;
   }
----------------
efriedma wrote:
> rnk wrote:
> > 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.
> We might be able to turn it into an indirect call...?  Not sure.
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.


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


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


================
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
----------------
efriedma wrote:
> rnk wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120622