This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] [z/OS] Add XPLINK64 Calling Convention to SystemZ
ClosedPublic

Authored by Everybody0523 on Apr 21 2021, 7:23 PM.

Details

Summary

This patch adds the XPLINK64 calling convention to the SystemZ backend. It specifies and implements the argument passing and return value conventions.

Diff Detail

Event Timeline

Everybody0523 created this revision.Apr 21 2021, 7:23 PM
Everybody0523 requested review of this revision.Apr 21 2021, 7:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2021, 7:23 PM

Formatting/style changes.

More style/format issues.

uweigand added inline comments.Apr 22 2021, 9:40 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
27

Please remove this unnecessary whitespace change here.

165–313

Why are you changing the sequence here? I didn't think the order of CSRs in that list should make any particular difference ...

176

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.

187

The last sentence doesn't seem to be correct -- according to the CSR definition above there are call-saved FPRs.

193

Space after ','.

198

Should we also provide more vector return registers for ABI extensions? Usually it should be fine to use all argument registers for returns too.

211

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?

248

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?

305

That last comment describes an action that doesn't appear to be done here, right?

Everybody0523 added inline comments.Apr 22 2021, 10:49 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
165–313

The CSR mask is generated in order based on this list. However, FPRs are preserved in the save area in reverse order, which is contrary to the current CSR mask. The previous commit not reversing the order of the callee-saved FPRs was an oversight.

With all that said and done, if you prefer I can keep the sequence unchanged for now and create a separate patch that fixes this bug.

uweigand added inline comments.Apr 22 2021, 11:00 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
165–313

If you need to save FPRs in some specific place in the save area for ABI reasons, that should be handled by code in SystemZFrameLowering.cpp (like is done for GPRs on Linux -- for FPRs there is no defined place where they need to be saved so it doesn't matter).

But in any case this is an independent question, so I agree that this should best be handled in a separate patch.

Kai added inline comments.Apr 22 2021, 11:59 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.h
188

Add . (dot) at end of sentence.

kpn added a subscriber: kpn.Apr 22 2021, 12:32 PM
kpn added inline comments.
llvm/lib/Target/SystemZ/SystemZCallingConv.td
193

What is this ABI non-compliant code that's getting support?

How does a long double complex get returned?

211

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

278

Don't you need to hedge a bit in this comment? Some FP arguments aren't passed in registers if there are too many integer arguments. (Unless I'm wrong and the restriction only applies to 32-bit XPLINK?)

Formatting issues in tablegen file + remove fix for unrelated issue.

Everybody0523 marked 2 inline comments as done.Apr 22 2021, 4:13 PM
Everybody0523 added inline comments.
llvm/lib/Target/SystemZ/SystemZCallingConv.td
278

If I am not mistaken you are referring to the requirement that a FP argument is within 15 words of the previous FP argument in the argument list?

If so, then yes - XPLINK64 has no such clause.

kpn added inline comments.Apr 23 2021, 5:54 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
278

That's the one, yes. It must be a 31-bit only restriction then. Noted. Thanks for the clarification!

kpn added inline comments.Apr 23 2021, 7:40 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
211

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

Added comment on how complex f128 types are passed. Added explanation on how floating point parameters could affect availability of GPRs for parameter passing. Added name of document that contains full xplink spec. Fixed some grammatical issues. Added CCIfExtend clause to XPLINK argument promotion.

Everybody0523 marked 7 inline comments as done.Apr 23 2021, 4:31 PM
Everybody0523 added inline comments.
llvm/lib/Target/SystemZ/SystemZCallingConv.td
248

The clang front-end should generate sext/zext for those types anyways so we should be using CCIfExtend as well. Thanks!

Everybody0523 marked an inline comment as done.Apr 23 2021, 4:31 PM

Add all param passing vector registers as return value regs too. Fix up some punctuation.

Everybody0523 marked 2 inline comments as done.Apr 27 2021, 11:10 AM
Everybody0523 added inline comments.
llvm/lib/Target/SystemZ/SystemZCallingConv.td
211

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.

uweigand added inline comments.Apr 28 2021, 5:03 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
211

I agree the current wording of the comment seems fine to me.

371

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

382

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

407

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.

uweigand added inline comments.Apr 28 2021, 5:55 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
193

Non ABI-compliant code here refers to LLVM front-ends other than clang, e.g. for languages other than C/C++ or for JIT use cases. Some of those use cases require multiple return values to be passed efficiently. While you could require any such users to define their own calling convention for that, this is quite tedious (and requires back-end changes). Therefore, it most LLVM back-ends provide an extension in their "default" calling convention for multiple return values, which is never used by LLVM IR generated by clang, but is available for these other use case.

However, if it is indeed true that in XPLINK, the C/C++ complex long double is already returned in four FPRs, then that would not could as "non ABI-compliant" use. But that's just a comment wording issue.

433

Note that the ELF calling convention makes special provisions for "short" vector types, i.e. those defined using attribute((vector_size(8))) or shorter. They may be passed in VRs like the "normal" 16 byte vectors, but if none are available they'll not get passed in a normal 16-byte stack slot, but rather in an 8-byte stack slot.

I notice that this provision is not in place with the XPLINK convention. Now I guess this is a decision for you to make, given that such types would be an extension to the ABI anyway. I just wanted to make sure that this is considered choice and not just happening by accident.

kpn added inline comments.May 3 2021, 10:51 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
211

Fair enough!

Remove special handling for i1, i8, i16 in calling convention. SDAG legalize types phase should already extend these, there is no need to add handling to extend these again.

Everybody0523 added inline comments.May 18 2021, 8:05 AM
llvm/lib/Target/SystemZ/SystemZCallingConv.td
382

Yes, that's the intention.

433

Hi Uli, we actually hadn't considered that - thank you for pointing this out.

After thinking about it for a while I think we should keep the current behavior, ie. pass in a 16-byte stack slot. The reason for this is if were to copy the ELF behavior, and pass short vector types in a 8-byte stack slot if passed on the stack, we would have to do one of two things.

  1. Have short vector types passed-by-register "shadow" 8 bytes in the argument area. However this would mean that vector registers can shadow a variable amount of space on the argument list depending of the type of the parameter contained within, which is very strange.
  2. Have short vector types passed-by-register "shadow" 16 bytes in the argument area, but those passed-by-stack occupy 8 bytes. This means that two params of the same type would occupy a different amount of memory in the argument list depending on whether it is passed-by-register or passed-by-stack - also very strange.
uweigand accepted this revision.May 18 2021, 8:35 AM

So it looks like this is good to go now. LGTM, thanks!

llvm/lib/Target/SystemZ/SystemZCallingConv.td
382

OK, thanks for confirming.

433

I see, that does make sense to me as well.

This revision is now accepted and ready to land.May 18 2021, 8:35 AM
This revision was landed with ongoing or failed builds.May 18 2021, 1:53 PM
This revision was automatically updated to reflect the committed changes.