[PATCH] D60348: [COFF, ARM64] Fix ABI implementation of struct returns

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

Hi Mandeep,

I spent a bit more time understanding the code in more depth now.
I have a few more remarks that I hope will help to make the code simpler and more understandable.
Overall, the design seems acceptable to me - especially as it is in line with how things are done in the X86 backend. It's good that we don't unnecessarily use more different ways to implement things.
One big remark though: I think this needs regression tests to be added that should cover the lines of code you're introducing in this patch.

I expect to be offline for the next 7 days, so if anyone else would want to review/approve an updated version of this patch: I'd also be happy with that if the above comments have been taken into account.



================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:19-23
+/// CCIfNotSubtarget - Match if the current subtarget doesn't has a feature F.
+class CCIfNotSubtarget<string F, CCAction A>
+    : CCIf<!strconcat("!static_cast<const AArch64Subtarget&>"
+                       "(State.getMachineFunction().getSubtarget()).", F),
+           A>;
----------------
Is CCIfNotSubtarget used anywhere? If not, this can be removed?


================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:44-46
+  // Note: For Win64, "inreg" indicates indirect returns.
+  // "inreg" not set: SRet is passed in X8, not X0 like a normal
+  // pointer parameter.
----------------
Based on my understanding of this code, I think the following comment would make it easier to understand, by explaining what is aimed for without making the comment only about windows systems:


```
// In AAPCS, an SRet is passed in X8, not X0 like a normal pointer parameter.
// However, on windows, in some circumstances, the SRet is passed in in X0 or X1 instead.
// The presence of the inreg attribute indicates that SRet is passed in the alternative register (X0 or X1), not X8:
//  - X0 for non-instance methods.
//  - X1 for instance methods.
```


================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:47-52
+  CCIfNotInReg<CCIfType<[i64],
+    CCIfSRet<CCIfType<[i64], CCAssignToRegWithShadow<[X8], [W8]>>>>>,
+
+  // "inreg" set: SRet is passed in X1 for instance methods.
+  CCIfInReg<CCIfType<[i64],
+    CCIfSRet<CCIfType<[i64], CCAssignToRegWithShadow<[X1], [W1]>>>>>,
----------------
Assuming the proposed extended comment/documentation is correct, I think it would be easier to follow the logic if "CCIfInReg" rather than "CCIfNotInReg" was used to express the logic (one negation less). Do you think that would be possible?


================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:47-52
+  CCIfNotInReg<CCIfType<[i64],
+    CCIfSRet<CCIfType<[i64], CCAssignToRegWithShadow<[X8], [W8]>>>>>,
+
+  // "inreg" set: SRet is passed in X1 for instance methods.
+  CCIfInReg<CCIfType<[i64],
+    CCIfSRet<CCIfType<[i64], CCAssignToRegWithShadow<[X1], [W1]>>>>>,
----------------
kristof.beyls wrote:
> Assuming the proposed extended comment/documentation is correct, I think it would be easier to follow the logic if "CCIfInReg" rather than "CCIfNotInReg" was used to express the logic (one negation less). Do you think that would be possible?
It seems to me that the case where the SRet parameter is passed in X0 isn't explicitly handled here.
Should it be?
If not, I think the comment above should be extended to explain how that case is handled elsewhere.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3211-3212
 
+  // Struct returns on Windows.
+  if (IsWin64) {
+    for (unsigned I = 0; I <= 1 && I < Ins.size(); ++I) {
----------------
I get the impression that this doesn't need to be protected by an "IsWin64" test.
The sret inreg combination will only be produced by a front-end when targeting windows. So maybe best to just test for "sret" and "inreg" attributes to alter behaviour, not also on the "IsWin64" predicate?

Or maybe this block of code is there to make sure that on Windows the Sret pointer does get returned (in X0). And there is no other way to get the information that the sret pointer must be returned, apart from there being an "sret inreg" function parameter present?
If that's the case, I think that extending the comment "Struct returns on Windows" to e.g. "On windows, Sret pointers must be returned, so record the pointer in a virtual register at the start of the function so it can be returned in the epilogue", makes the intent of this code block a bit easier to understand. Having looked into X86ISelLowering for equivalent functionality, I saw the following words were used in a comment there, that I thought was especially clear: "Save the argument into a virtual register so that we can access it from the return points.".



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3217-3225
+      if (Ins[I].Flags.isInReg() || I == 1) {
+        // If isInReg: "sret inreg" on a parameter indicates a struct
+        // return in X0.
+
+        // If I == 1: Meaning this is the second parameter to the function. If
+        // the second parameter is marked "sret", it means the first parameter
+        // is "this".  This is how we identify instance methods. In this case,
----------------
Assuming my understanding on the intent of this code is correct (i.e. "Save the argument into a virtual register so that we can access it from the return points."), there is no need to make a distinction between whether the sret pointer is passed in in X0 or X1 - it needs to be returned via X0 in both cases.
So, why not just have a very simple loop here that looks for the presence of an argument with both "sret" and "inreg" attributes and save the value of that argument in the getSRetReturnReg?
I think the loop could look almost identical to how similar functionality is implemented in X86ISelLowering with quite a bit less code - making this quite a bit easier to understand and maintain?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:4001-4004
+  // Struct returns on Windows.
+  bool IsWin64 =
+    Subtarget->isCallingConvWin64(MF.getFunction().getCallingConv());
+  if (IsWin64) {
----------------
Similar comment to the above: maybe it's not necessary to check for IsWin64 here - just checking on whether getSRetReturnReg is set could be enough?
It would make the code less complex.
The comment could be extended a bit to make this a bit more clear - I thought the equivalent comment on X86IselLowering was pretty clear. Maybe something like:

```
  // Windows AArch64 ABIs require that for returning structs by value we copy
  // the sret argument into X0 for the return.
  // We saved the argument into a virtual register in the entry block,
  // so now we copy the value out and into X0.
```


================
Comment at: lib/Target/AArch64/AArch64MachineFunctionInfo.h:94-98
+  /// SRetReturnReg - sret lowering includes returning the value of the
+  /// returned struct in a register. This field holds the virtual register into
+  /// which the sret argument is passed.
+  unsigned SRetReturnReg = 0;
+
----------------
mgrang wrote:
> kristof.beyls wrote:
> > It seems this is also present in X86MachineFunctionInfo.
> > I wonder if it wouldn't be better to move this to the MachineFunctionInfo base class so we don't need to add an identical interface in 2 derived classes?
> MachineFunctionInfo base only has a factory function for creating objects. All fields/methods are in derived classes. So not sure if we should move SRetReturnReg  to the base.
I see - thanks for explaining!


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

https://reviews.llvm.org/D60348