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

Wed Apr 28 05:03:50 PDT 2021

uweigand added inline comments.

================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:224
+  // before placing the parameters either on the stack or in registers
+  CCIfType<[i1, i8, i16, i32], CCIfExtend<CCPromoteToType<i64>>>,
+
----------------
One more thing I just noticed here  (b.t.w. applies also to the return value convention):  I don't think we can ever end up with i1, i8, or i16 here, because those types are not "legal" in the SystemZ back-end, so they will already have been converted to i32 by common code (type legalization).  That is why the Linux ABI code only checks for i32 here.

I **think** this would still hold true for the XPLINK convention -- if so, it would be preferable to remove the small types here as well.  (This should have no effect on codegen, just remove an unnecessary difference between the code for the two calling conventions.)

================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:235
+  // We need to deal with this first
+  CCIfType<[i64], CCCustom<"CC_SystemZ_I128Indirect">>,
+  // The first 3 integer arguments are passed in registers R1D-R3D.
----------------
Just to re-confirm: this handler will pass an i128 via implicit pointer (i.e. push the i128 to the stack somewhere outside the argument list, and pass a pointer to that location as argument).   Is that actually correct for XPLINK?  Reading the specs in the documentation referred to above, I get the impression that XPLINK never passes arguments via implicit pointers ...

================
Comment at: llvm/lib/Target/SystemZ/SystemZCallingConv.td:260
+  // and FPR4/FPR6. The rest are passed on the stack.
+  // Unless the arguments are passed as VARARGS.
+  CCIfType<[f128], CCIfFixed<CCCustom<"CC_XPLINK64_Shadow_Reg">>>,
----------------
One more comment wording nit-pick:  I'd say "The first 2 named long double arguments" just like the in previous two clauses, and then remove the "Unless ..." sentence.    We should just the same wording for the same concept across those different clauses.

================
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
----------------
Everybody0523 wrote:
> kpn wrote:
> > kpn wrote:
> > > uweigand wrote:
> > > > 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?
> > > The comment seems misleading. Integer types get expanded to fill a whole GPR. So passing three chars uses 3 registers -> 24 bytes. Talking about bytes therefore doesn't seem useful. Worse, it's misleading to refer to "bytes" for GPR and not for the FPR since it implies that parms get packed or something. I see that you addressed this below, but it'd be better to not even provoke the thought in someone reading the code. I'd stick with "registers" instead of "bytes" in this sentence.
> > > 
> > > One has to look closely at the table to see that FP parms can prevent a GPR from being used that would be used if there were no FP parms. So that bit could use a description in english. 
> > > 
> > > A link to the ABI doc would be useful, or at least the name of it. Since links go stale, the name is probably a minimum.
> > > 
> > > Actually, how about copying the tables that are (were?) in the ABI documentation? Last time I looked IBM had a list of examples showing how FP values can push around GPR values, and how large numbers of integer parms can prevent FP values from being passed in FP registers despite FP registers being available. (I admit it's been years since I looked at the documentation.)
> > Now that I've looked at the current version of Vendor Interfaces, the table I was thinking of is in Appendix B, from pages 924 to 931. That's ... a lot.
> > 
> > Oh, and appendix B doesn't include the documentation that is elsewhere in the book. XPLINK calling conventions require that much documentation. So a reference to the documentation may be better than trying to duplicate it.
> > 
> > Ulrich, how about this: Instead of having this table below, and instead of trying to put 6-7 pages of documentation in comments here, how about a comment about how there are a number of common tricky cases and they're documented in <documentation bib entry here>?
> I've updated the comments to include a note on when the full specification of the ABI can be found. 
> 
> I am hesitant to include a link for reason you mentioned above - links can go stale. Given the name of the document, it shouldn't be hard to find the latest version of the LE vendor interfaces.
I agree the current wording of the comment seems fine to me.

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

https://reviews.llvm.org/D101010