[PATCH] D133484: [AArch64][SVE] Fix AArch64_SVE_VectorCall calling convention

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6750
+      if (ArgLocs[i].isRegLoc()) {
+        if (EVT RegVT = ArgLocs[i].getLocVT(); RegVT.isScalableVector())
+          CallConv = CallingConv::AArch64_SVE_VectorCall;
----------------
Not sure if there's a coding standard rule against this but it's not the typically style used.  Most would write:
```
if (EVT RegVT = ArgLocs[i].getLocVT())
  if (RegVT.isScalableVector())
    CallConv = CallingConv::AArch64_SVE_VectorCall;
```
That said I think relying on the ValueType is fragile.  Given ArgLocs contains actual register assignments would the following also work instead of the original lambda function?
```
if (!Loc.isRegLoc())
  return false;

return AArch64::ZPRRegClass.contains(Loc.getLocReg()) || AArch64::PPRRegClass.contains(Loc.getLocReg());
```




================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6754
+    }
     bool CalleeInSVE = any_of(Ins, [](ISD::InputArg &In){
       return In.VT.isScalableVector();
----------------
peterwaller-arm wrote:
> I can't think of a way that you can get a returned scalable type which doesn't go in a register, but it doesn't seem inconceivable it may be possible in the distant future. Since we're leaving a subtlety here it seems worth a brief comment of this form:
> 
>   // Check the types of the returned values. For the time being it is sufficient to check the VT as the RegVT is not known at this point.
Can we handle this in the same way as we do for arguments. The necessary work is done within `LowerCallResult`. It looks like every target does something special with `LowerCall` our only caller so I think we should hoist the necessary logic to construct `RVLocs` into `LowerCall` and pass it into `LowerCallResult` instead of `Ins`.

Doing this means we can use the same lambda as  above to give us a bit of future proofness (I don't expect extra test coverage because I don't believe this is all that well defined given there's no way to write a suitable example in C). 


================
Comment at: llvm/test/CodeGen/AArch64/sve-calling-convention-mixed.ll:393
 
 define <vscale x 4 x float> @sve_caller_non_sve_callee_high_range(<vscale x 4 x float> %v0, <vscale x 4 x float> %v1)  {
 ; CHECK-LABEL: sve_caller_non_sve_callee_high_range:
----------------
Given the context of the patch I think you could do with a few more tests to ensure all the code paths are covered. For example:
```
non_sve_caller_non_sve_high_range_callee
non_sve_high_range_caller_non_sve_high_range_callee
sve_caller_non_sve_high_range_callee - this is the function you have today
sve_ret_caller_non_sve_high_range_callee - as above but with no sve arguments
```

It's also worth adding another `aapcs#` callee test whereby the callee has an out of range SVE argument but returns an SVE result.


================
Comment at: llvm/test/CodeGen/AArch64/sve-calling-convention-mixed.ll:490
 ; CHECK-NEXT:    ret
   call void @non_sve_callee_high_range(float 0.0, float 1.0, float 2.0, float 3.0, float 4.0, float 5.0, float 6.0, float 7.0, <vscale x 4 x float> %v0, <vscale x 4 x float> %v1)
   ret <vscale x 4 x float> %v0
----------------
It's better for this to be a declared function only so there's no chance of something peeking into the called function. If `non_sve_callee_high_range` serves no other purpose then you can just delete its definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133484