[PATCH] D101010: [SystemZ] [z/OS] Add XPLINK64 Calling Convention to SystemZ

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:27
 
-
 //===----------------------------------------------------------------------===//
----------------
Please remove this unnecessary whitespace change here.


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:165
 def CSR_SystemZ_XPLINK64 : CalleeSavedRegs<(add (sequence "R%dD", 8, 15),
-                                                (sequence "F%dD", 8, 15))>;
+                                                (sequence "F%dD", 15, 8))>;
+
----------------
Why are you changing the sequence here?  I didn't think the order of CSRs in that list should make any particular difference ...


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:176
+  // XPLINK64 ABI compliant code
+  // widens i1, i8, i16, i32 to i64
+  CCIfType<[i1, i8, i16, i32], CCPromoteToType<i64>>,
----------------
Please revisit the comments here and below and look for wording and formatting issues.   This comment would fit on one line and needs a '.' and the end.


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:187
+  // and FPR6 are also provided for ABI non-compliant code.
+  // All floating point register are call-clobbered
+  CCIfType<[f32], CCAssignToReg<[F0S, F2S, F4S, F6S]>>,
----------------
The last sentence doesn't seem to be correct -- according to the CSR definition above there are call-saved FPRs.


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:193
+  // F4D and F6D, hence F4Q, are provided for ABI non-compliant code.
+  CCIfType<[f128], CCAssignToReg<[F0Q,F4Q]>>,
+
----------------
Space after ','.


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:198
+    CCIfType<[v16i8, v8i16, v4i32, v2i64, v4f32, v2f64],
+             CCAssignToReg<[V24]>>>
+]>;
----------------
Should we also provide more vector return registers for ABI extensions?  Usually it should be fine to use all argument registers for returns too.


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:211
+// space must be reserve for all the args on stack
+
+// 1           2           3              (register used, the 3 arg
----------------
If you want to have a comment here describing how the ABI works in general terms, that's of course good, but I'm not sure if the long table here is particularly useful.  Maybe describing it in terms of rules would be better?   Or, even better, providing a link to an offical ABI doc?


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:248
+  // before placing the parameters either on the stack or in registers
+  CCIfType<[i1, i8, i16, i32], CCPromoteToType<i64>>,
+
----------------
So a difference to Linux ABI code here is that in Linux we use CCIfExtend, i.e. the extension only happens if the argument is marked using "sext" or "zext" in the IR (which gets set from clang depending on the source type, or omitted e.g. if we're passing a struct in register that cannot be "extended" as such).    This code here calls CCPromoteToType unconditionally, so you'll get AnyExtend nodes in cases where there is no "sext" or "zext" marker.  Is this intended?


================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:305
+  // Stack space must also be assigned for reg vars in the argument area,
+  // even though they are not saved on stack
+  CCIfType<[i32, i64, f32, f64], CCAssignToStack<8, 8>>,
----------------
That last comment describes an action that doesn't appear to be done here, right?


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

https://reviews.llvm.org/D101010