This is an archive of the discontinued LLVM Phabricator instance.

Add support for System z vector language extensions
ClosedPublic

Authored by uweigand on Jul 7 2015, 10:06 AM.

Details

Summary

The z13 vector facility has an associated language extension,
closely modeled on AltiVec/VSX. The main differences are:

  • vector long, vector float and vector pixel are not supported
  • vector long long and vector double are supported (like VSX)
  • comparison operators return a vector rather than a scalar integer
  • shift operators behave like the OpenCL shift operators
  • vector bool is only supported as argument to certain operators; some operators allow mixing a bool with a non-bool vector

This patch adds clang support for the extension. It is closely modelled
on the AltiVec support. As with AltiVec, there's a separate -m option
(-mzvector) that is only supported for z (just as -maltivec is only
supported for PowerPC). There's also a separate LangOpt.

The extension as implemented here is intended to be compatible with
the -mzvector extension recently implemented by GCC.

Based on a patch by Richard Sandiford.

Diff Detail

Repository
rL LLVM

Event Timeline

uweigand updated this revision to Diff 29188.Jul 7 2015, 10:06 AM
uweigand retitled this revision from to Add support for System z vector language extensions.
uweigand updated this object.
uweigand added reviewers: rsmith, hfinkel.
uweigand added a subscriber: cfe-commits.
rsmith edited edge metadata.Jul 7 2015, 2:01 PM

Is this extension documented anywhere?

include/clang/Basic/LangOptions.def
107 ↗(On Diff #29188)

Should this "z" be uppercase (here and elsewhere in the patch)? Currently in Clang's comments we always spell this "SystemZ" (with no space); we should ideally only have a single typographical convention for this.

include/clang/Driver/Options.td
1330–1333 ↗(On Diff #29188)

This should be a -f flag, not a -m flag. (I think we only support -maltivec for GCC compatibility.)

lib/CodeGen/CGExprScalar.cpp
2859–2860 ↗(On Diff #29188)

The code after this if will not do the right thing in this case. Why do you need this change? What are the semantics of a vector comparison that produces a scalar under this extension?

lib/Driver/Tools.cpp
3955–3959 ↗(On Diff #29188)

What is the reason for this restriction? Would the extension not work in some way on other architectures? For -faltivec, we use ppc_altivec_vcmp* intrinsics for some comparisons (and even that is tenuous justification, because these intrinsics could be represented directly in IR), but there doesn't seem to be anything comparable in this case?

uweigand marked 3 inline comments as done.Jul 8 2015, 5:38 AM

Is this extension documented anywhere?

There is no formal documentation for the Linux version of the extension, but it is closely modeled on the corresponding extension that was introduced by the IBM XL C/C++ compiler for z/OS. That version is documented here:
http://www-01.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.cbcpx01/vectorsupport.htm

The implementation of the vector extension for Linux on z in GCC followed the z/OS version except for a few minor differences (in order to improve compatibility with the GCC vector extension). This LLVM implementation in turn follows the GCC implementation.

include/clang/Basic/LangOptions.def
107 ↗(On Diff #29188)

The official name for the family of IBM mainframes is "System z" with space and lower-case letter. (Actually, it was recently renamed to "z Systems", but that's probably a different issue ...)

The name of the LLVM target has always been "SystemZ" without space and with upper-case letter. I'm not sure where the difference comes from, but I assume this is due to fact that we don't want spaces in filenames, and without space, a lower-case "z" would have looked weird ...

This difference between the machine name and the LLVM target name is a bit unfortunate, but I guess not unprecedented (e.g. the Intel target is called "X86" in LLVM, but the official name uses a lower-case letter).

I've tried to keep consistent with existing uses and use SystemZ for internal comments refering to the LLVM target name, but System z in user-visible messages refering to the machine name.

include/clang/Driver/Options.td
1330–1333 ↗(On Diff #29188)

OK, I've changed this to a -f flag, and added -mzvector as alias for GCC compatibility.

lib/CodeGen/CGExprScalar.cpp
2859–2860 ↗(On Diff #29188)

You're right, this code is not needed. In this extension, the result of a vector comparison is always a vector, not a scalar. I've removed this code.

lib/Driver/Tools.cpp
3955–3959 ↗(On Diff #29188)

No particular reason, just because it was done for Altivec too ... The extension as such should work fine on other architectures. I've removed the restriction now.

uweigand updated this revision to Diff 29256.Jul 8 2015, 5:41 AM
uweigand edited edge metadata.
uweigand marked 3 inline comments as done.

Adressed review comments:

  • Use -fzvector flag (plus -mzvector as alias)
  • Removed incorrect change in CGExprScalar.cpp
  • Removed restriction to System z architecture
rsmith added a comment.Jul 8 2015, 5:06 PM

I think it's reasonable for Clang to natively support this extension [especially since GCC and xlC support this, there is presumably a significant amount of existing code that uses this extension on System z, there is a lot of overlap with our existing Altivec extension, and we have a code owner with a strong track record proposing it].

The patch looks to be in really good shape. (FWIW, I find it a bit weird that vector long is not valid but all the other vectors of integral types are, but hey, it's your extension...)

lib/Driver/Tools.cpp
3955–3959 ↗(On Diff #29256)

Thanks.

We should probably reject -faltivec -fzvector, since they give different semantics for certain vector operations. (Either that, or we need to pick which one wins in each case, which doesn't sound great.)

lib/Sema/SemaExpr.cpp
7430 ↗(On Diff #29256)

It'd be better to phrase this as a positive language mode check than a negative one (that is, AllowbothBool = getLangOpts().Altivec) -- generally, where possible, we should aim for LangOptions values to enable features rather than disable them.

uweigand marked 2 inline comments as done.Jul 9 2015, 8:50 AM

I think it's reasonable for Clang to natively support this extension [especially since GCC and xlC support this, there is presumably a significant amount of existing code that uses this extension on System z, there is a lot of overlap with our existing Altivec extension, and we have a code owner with a strong track record proposing it].

Thanks!

The patch looks to be in really good shape. (FWIW, I find it a bit weird that vector long is not valid but all the other vectors of integral types are, but hey, it's your extension...)

AltiVec has allowed "vector long", but this has always been causing issues when writing code supporting both 32-bit and 64-bit mode, since the base "long" type changes size. Should "vector long" be four 32-bit or two 64-bit values, or should that change depending on compilation mode? Either choice causes surprises to the user in some circumstances ...

Because of that, "vector long" has been considered "deprecated" for AltiVec for at least 10 years now, and the recommendation is to use either "vector int" or else "vector long long". For the new System z extension, we therefore started out with "vector long" not just deprecated, but unsupported. (For clang, this is not so much of an issue since we currently support only 64-bit mode on System z, but both GCC and XLC do support 32-bit mode on System z as well, so there's the same issue there.)

lib/Driver/Tools.cpp
3955–3959 ↗(On Diff #29256)

Good point. I've added code to reject the combination.

lib/Sema/SemaExpr.cpp
7430 ↗(On Diff #29256)

OK, updated accordingly.

uweigand updated this revision to Diff 29337.Jul 9 2015, 8:53 AM
uweigand marked 2 inline comments as done.

Adressed review comments:

  • Reject combination of both -fzvector and -faltivec
  • Use AllowBothBool = LangOpts.AltiVec instead of !LangOpts.ZVector

Is this OK to check in now?

If at all possible, I'd really like to get this in before 3.7 branches, so that we can claim full z13 support with the release ...

rsmith accepted this revision.Jul 29 2015, 6:37 PM
rsmith edited edge metadata.

LGTM, sorry for the delay. Please remove the -mzvector flag if we don't actually need it for GCC compatibility.

include/clang/Driver/Options.td
1330–1333 ↗(On Diff #29337)

Does GCC really support -mzvector? I can't find any reference to such a flag, and grepping the GCC sources finds no occurrences of the string "zvector" at all.

include/clang/Parse/Parser.h
115 ↗(On Diff #29337)

Too many "cached"s

This revision is now accepted and ready to land.Jul 29 2015, 6:37 PM
uweigand marked an inline comment as done.Jul 30 2015, 7:07 AM

LGTM, sorry for the delay. Please remove the -mzvector flag if we don't actually need it for GCC compatibility.

Thanks for the review! We do in fact need the -mzvector flag for GCC compatibility, see below.

include/clang/Driver/Options.td
1330–1333 ↗(On Diff #29337)

The flag is implemented here in the GCC sources:
https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/s390/s390.opt?revision=223398&view=markup

mzvector
Target Report Mask(ZVECTOR)
Enable the z vector language extension providing the context-sensitive vector macro.

(You're right that this flag currently does not show up in the GCC documentation. That was an oversight. I've asked Andreas to fix this.)

include/clang/Parse/Parser.h
115 ↗(On Diff #29337)

Fixed.

This revision was automatically updated to reflect the committed changes.
uweigand marked an inline comment as done.
cfe/trunk/lib/Sema/SemaLookup.cpp