[cfe-commits] [PATCH][Review request] correct initialization of AltiVec vectors
Douglas Gregor
dgregor at apple.com
Thu Feb 3 07:16:11 PST 2011
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
More information about the cfe-commits
mailing list