[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:       Richard Smith <richard () metafoo ! co ! uk>
Date:       2015-07-07 21:01:07
Message-ID: a18788d27f514e2d635dbd21e1308f9e () localhost ! localdomain
[Download RAW message or body]

Is this extension documented anywhere?


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

================
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]>;
+
----------------
This should be a `-f` flag, not a `-m` flag. (I think we only support `-maltivec` 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
----------------
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?

================
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";
+
----------------
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?


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]