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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D60348#1466139 <https://reviews.llvm.org/D60348#1466139>, @kristof.beyls wrote:

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


Fair enough, I just got back to work, so I'll look into it. :)

>From what I can tell, the last update implements all the suggested simplifications. I have a suggestion, but maybe it cuts against the direction you were suggesting.

Overall, I'm happy with this.



================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:39-51
+  // 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 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.
+
----------------
I feel like the whole discussion of free function vs. C++ instance method (X0/X1) could be avoided by falling through to the normal i64 argument handling below. IIUC, these are roughly the conditions we want:
  if (IsSRet && (IsInReg || !IsWin64))
    // assign to X8
I'm not sure how to express that in our calling conv tablegen, though.


================
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;
+
----------------
kristof.beyls wrote:
> 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!
If we did want to avoid this duplication, I would say just toss it directly on MachineFunction. But, for now, I think it's best to do things like we normally do: duplicate some logic between targets but try to keep them aligned.


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

https://reviews.llvm.org/D60348