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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

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.



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


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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5972
         (!TT.isOSWindows() || TT.isOSBinFormatELF() || TT.isOSBinFormatMachO()))
       return false;
   }
----------------
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.


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


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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6074
   const MachineRegisterInfo &MRI = MF.getRegInfo();
   if (!parametersInCSRMatch(MRI, CallerPreserved, ArgLocs, OutVals))
     return false;
----------------
rnk wrote:
> 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?
This naturally shouldn't fail for musttail calls, I think.


================
Comment at: llvm/test/CodeGen/AArch64/darwinpcs-musttail.ll:17
+  ret void
+}
----------------
Can you add a plain "tail" testcase in addition to the "musttail" testcase, since this patch affects the behavior of that too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120622