[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-09 15:50:44
Message-ID: 8c70a347e2676aef7c1c2309ecece948 () localhost ! localdomain
[Download RAW message or body]

uweigand marked 2 inline comments as done.
uweigand added a comment.

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

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


================
Comment at: lib/Driver/Tools.cpp:3955-3959
@@ -3952,7 +3954,7 @@
 
   if (getToolChain().SupportsProfiling())
     Args.AddLastArg(CmdArgs, options::OPT_pg);
 
   // -flax-vector-conversions is default.
   if (!Args.hasFlag(options::OPT_flax_vector_conversions,
                     options::OPT_fno_lax_vector_conversions))
----------------
rsmith wrote:
> 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.)
Good point.  I've added code to reject the combination.

================
Comment at: lib/Sema/SemaExpr.cpp:7430
@@ +7429,3 @@
+    return CheckVectorOperands(LHS, RHS, Loc, IsCompAssign,
+                               /*AllowBothBool*/!getLangOpts().ZVector,
+                               /*AllowBoolConversions*/false);
----------------
rsmith wrote:
> 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.
OK, updated accordingly.


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]