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

[Anton Yartsev]

On 16.02.2011 13:29, 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!
>
Ping!

-- 
Anton