[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D11001: Add support for System z vector language extensions
From:       Ulrich Weigand <ulrich.weigand () de ! ibm ! com>
Date:       2015-07-08 12:38:26
Message-ID: 6464dd0246432af7cc416f611371fbf6 () localhost ! localdomain
[Download RAW message or body]

uweigand marked 3 inline comments as done.

In http://reviews.llvm.org/D11001#200688, @rsmith wrote:

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


================
Comment at: include/clang/Basic/LangOptions.def:107
@@ -106,2 +106,3 @@
 LANGOPT(AltiVec           , 1, 0, "AltiVec-style vector initializers")
+LANGOPT(ZVector           , 1, 0, "System z vector extensions")
 LANGOPT(Exceptions        , 1, 0, "exception handling")
----------------
rsmith wrote:
> 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.
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.

================
Comment at: include/clang/Driver/Options.td:1330-1333
@@ -1329,1 +1329,6 @@
 
+def mzvector : Flag<["-"], "mzvector">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Enable System z vector language extension">;
+def mno_zvector : Flag<["-"], "mno-zvector">, Group<m_Group>,
+  Flags<[CC1Option]>;
+
----------------
rsmith wrote:
> This should be a `-f` flag, not a `-m` flag. (I think we only support `-maltivec` for GCC \
> compatibility.)
OK, I've changed this to a -f flag, and added -mzvector as alias for GCC compatibility.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:2859-2860
@@ -2858,3 +2858,4 @@
     // intrinsics comparing vectors and giving 0 or 1 as a result
-    if (LHSTy->isVectorType() && !E->getType()->isVectorType()) {
+    if (!CGF.getLangOpts().ZVector &&
+        LHSTy->isVectorType() && !E->getType()->isVectorType()) {
       // constants for mapping CR6 register bits to predicate result
----------------
rsmith wrote:
> 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?
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.

================
Comment at: lib/Driver/Tools.cpp:3955-3959
@@ -3952,1 +3954,7 @@
 
+  // Similarly for -mzvector and SystemZ.
+  if (const Arg *A = Args.getLastArg(options::OPT_mzvector))
+    if (!(getToolChain().getArch() == llvm::Triple::systemz))
+      D.Diag(diag::err_drv_argument_only_allowed_with)
+        << A->getAsString(Args) << "systemz";
+
----------------
rsmith wrote:
> 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?
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.


http://reviews.llvm.org/D11001




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]