[cfe-commits] [PATCH][Review request] correct initialization of AltiVec vectors

[Douglas Gregor]

On Jan 25, 2011, at 1:45 PM, Anton Yartsev wrote:

> On 17.01.2011 21:59, Douglas Gregor wrote:
>> On Dec 29, 2010, at 4:14 PM, Anton Yartsev wrote:
>> 
>>> Hi all,
>>> 
>>> made changes according to the rules for the '(...)' form of vector initialization in AltiVec: the number of initializers must be one or must match the size of the vector. If a single value is specified in the initializer then it will be replicated to all the components of the vector.
>>> 
>>> Can I commit the patch?
>> +    // '(...)' form of vector initialization in AltiVec: the number of
>> +    // initializers must be one or must match the size of the vector.
>> +    // If a single value is specified in the initializer then it will be
>> +    // replicated to all the components of the vector
>> +    if (Ty->getAs<VectorType>()->getVectorKind() ==
>> +        VectorType::AltiVecVector) {
>> +      unsigned numElems = Ty->getAs<VectorType>()->getNumElements();
>> +      // The number of initializers must be one or must match the size of the
>> +      // vector. If a single value is specified in the initializer then it will
>> +      // be replicated to all the components of the vector
>> +      if (PE->getNumExprs() == 1) {
>> +        Expr* initExpr = PE->getExpr(0);
>> +        for (unsigned i = 0; i != numElems; ++i)
>> +          initExprs.push_back(initExpr);
>> 
>> This is effectively just a vector splat, so why not model it as an explicit C-style cast with kind CK_VectorSplat? That would more accurately describe what's happening than making this look like a vector initialization.
>> 
>> 	- Doug
> Done!

A few comments:

Index: lib/Sema/SemaCXXCast.cpp
===================================================================
--- lib/Sema/SemaCXXCast.cpp	(revision 124051)
+++ lib/Sema/SemaCXXCast.cpp	(working copy)
@@ -1227,7 +1227,16 @@

     // FIXME: Should this also apply to floating point types?
     bool srcIsScalar = SrcType->isIntegralType(Self.Context);
     bool destIsScalar = DestType->isIntegralType(Self.Context);
-    
+
+    // Case of AltiVec vector initialization with a single literal
+    if (destIsVector
+        && DestType->getAs<VectorType>()->getVectorKind() ==
+           VectorType::AltiVecVector
+        && (SrcType->isIntegerType() || SrcType->isFloatingType())) {
+      Kind = CK_VectorSplat;
+      return TC_Success;
+    }
+
     // Check if this is a cast between a vector and something else.
     if (!(srcIsScalar && destIsVector) && !(srcIsVector && destIsScalar) &&
         !(srcIsVector && destIsVector))

What's your motivation for this change? This is along the reinterpret_cast path, and reinterpret_cast is meant only for conversions that take the same bits and interpret them in a different way. A vector splat isn't such a conversion.

This change basically ends up taking something that used to work in a reinterpret_cast---say, reinterpreting an int as a vector of 4 chars---and introduces an conceptual ambiguity (it could also mean a vector splat), then resolves it to the vector splat. Is this really what you intended?

@@ -4885,18 +4893,45 @@

     }
     if (PE->getNumExprs() == 1) {
       if (!PE->getExpr(0)->getType()->isVectorType())
-        isAltiVecLiteral = true;
+        isVectorLiteral = true;
     }
     else
-      isAltiVecLiteral = true;
+      isVectorLiteral = true;
   }
 
-  // If this is an altivec initializer, '(' type ')' '(' init, ..., init ')'
+  // If this is a vector initializer, '(' type ')' '(' init, ..., init ')'
   // then handle it as such.
-  if (isAltiVecLiteral) {
+  if (isVectorLiteral) {
     llvm::SmallVector<Expr *, 8> initExprs;
-    for (unsigned i = 0, e = PE->getNumExprs(); i != e; ++i)
-      initExprs.push_back(PE->getExpr(i));
+    // '(...)' form of vector initialization in AltiVec: the number of
+    // initializers must be one or must match the size of the vector.
+    // If a single value is specified in the initializer then it will be
+    // replicated to all the components of the vector
+    if (Ty->getAs<VectorType>()->getVectorKind() ==
+        VectorType::AltiVecVector) {
+      unsigned numElems = Ty->getAs<VectorType>()->getNumElements();
+      // The number of initializers must be one or must match the size of the
+      // vector. If a single value is specified in the initializer then it will
+      // be replicated to all the components of the vector
+      if (PE->getNumExprs() == 1) {
+        QualType ElemTy = Ty->getAs<VectorType>()->getElementType();
+        Expr *Literal = PE->getExpr(0);
+        ImpCastExprToType(Literal, ElemTy,
+                          PrepareScalarCast(*this, Literal, ElemTy));
+        return BuildCStyleCastExpr(LParenLoc, TInfo, RParenLoc, Literal);
+      }

This looks good. One question: if we have a one-element vector, shouldn't this be handled as an initializer of that one-element vector rather than a splat? I expect that code generation will be roughly the same, but it would be cleaner to represent this as an initialization. 

There seem to be a bunch of whitespace changes in the diff for test/CodeGen/builtins-ppc-altivec.c, which make it hard to see any actual changes.

> This solution, however, does not work in global scope for some reason. Investigating the problem.

Okay, looking forward to an updated patch.

	- Doug