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 [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
