[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 had a quick look through and wrote up some thoughts that triggered immediately.
I'll try to have a further look when I find more time.

Thanks!



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3212
+  // Struct returns on Windows.
+  // Note: This has been based on X86 implementation.
+  if (IsWin64) {
----------------
As said elsewhere, I don't think the note is that helpful after this gets committed.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3219
+      if (Ins[I].Flags.isInReg() || I == 1) {
+        // If isInReg: "sret inreg" on a parameter indicates a non-POD struct
+        // return. And if these attributes are on param0 then callee needs to
----------------
I wonder if the a more exact word could be used than "non-POD"?
The updated documentation at https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values  doesn't describe the rules in terms of POD anymore. I wonder if a more exact term could be used here than "non-POD"?  I realize that may be hard... Maybe just drop the word "non-POD", as it probably wouldn't remove anything from the clarity of the explanation in the comment?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3223
+
+        // If I == 1: If param1 is marked "sret", it means param0 is "this".
+        // This is how we identify instance methods. In this case, the
----------------
I needed to browse around the surrounding code quite a bit to understand what "I == 1" really means.
I guess it means "this is the second argument to the function". If my guess is correct, maybe it'd be useful to spell it out in the comment?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3226
+        // address of the struct is passed in X1. So in the function epilog
+        // we need to copy X1 to X0.
+
----------------
I wonder if it'd be a good idea to add a pointer in the comment to the relevant MSVC ABI documentation indicating that the struct pointer must be returned in X0?
I think that would be https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:4007
+  // Struct returns on Windows.
+  // Note: This has been based on X86 implementation.
+  bool IsWin64 =
----------------
This note is helpful for review, indicating it's based on X86 implementation. However, I don't think it adds too much value if it'd be committed as is to the code base.


================
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;
+
----------------
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?


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

https://reviews.llvm.org/D60348