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

[Douglas Gregor]

On Feb 16, 2011, at 2:29 AM, Anton Yartsev wrote:

> 
>> 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!


I definitely prefer the VectorSplat cast approach. One question about the patch:

Index: lib/CodeGen/CGExprConstant.cpp

===================================================================
--- lib/CodeGen/CGExprConstant.cpp	(revision 124983)
+++ lib/CodeGen/CGExprConstant.cpp	(working copy)
@@ -907,12 +907,29 @@

       llvm::SmallVector<llvm::Constant *, 4> Inits;
       unsigned NumElts = Result.Val.getVectorLength();
 
-      for (unsigned i = 0; i != NumElts; ++i) {
-        APValue &Elt = Result.Val.getVectorElt(i);
-        if (Elt.isInt())
-          Inits.push_back(llvm::ConstantInt::get(VMContext, Elt.getInt()));
-        else
-          Inits.push_back(llvm::ConstantFP::get(VMContext, Elt.getFloat()));
+      if (Context.getLangOptions().AltiVec &&
+          isa<CStyleCastExpr>(E) &&
+          dyn_cast<CStyleCastExpr>(E)->getCastKind() == CK_VectorSplat) {

Why check for a C-style cast here? Why not just check for a (general) CastExpr, which would also cover C-style casts (e.g., static_cast) and implicit casts?

Also, one little nit: if you've already done an isa<T>(X), you can just cast<T>(X) rather than dyn_cast<T>(X), and save ourselves some checking.

Sorry for the long-delayed review :(

	- Doug