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

[Anton Yartsev]

> 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?
>
That was the wrong place for the code, moved to the right one

> @@ -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 are no one-element vector types in AltiVec.

Initialization is cleaner, but it gives a little bit complicated 
bitcode. Transforming the following code

int main() {
   vector int v = (vector int)(1);
   return 0;
}

with the solution using initialization we obtain

define i32 @main() nounwind {
entry:
   %retval = alloca i32, align 4
   %v = alloca <4 x i32>, align 16
   %.compoundliteral = alloca <4 x i32>, align 16
   store i32 0, i32* %retval
   store <4 x i32> <i32 1, i32 1, i32 1, i32 1>, <4 x i32>* 
%.compoundliteral
   %tmp = load <4 x i32>* %.compoundliteral
   store <4 x i32> %tmp, <4 x i32>* %v, align 16
   ret i32 0
}

and with the solution using cast -

define i32 @main() nounwind {
entry:
   %retval = alloca i32, align 4
   %v = alloca <4 x i32>, align 16
   store i32 0, i32* %retval
   store <4 x i32> <i32 1, i32 1, i32 1, i32 1>, <4 x i32>* %v, align 16
   ret i32 0
}

> 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 are not whitespace changes - the bitcode changed

Attached are fixed patches performed one as cast and another as 
initialization, please review and chose!

-- 
Anton

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: AltiVec_vector_init_fix_CAST.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110216/8652c4c4/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: AltiVec_vector_init_fix_INITIALIZATION.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110216/8652c4c4/attachment-0001.ksh>