[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]