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

[Douglas Gregor]

On Mar 18, 2011, at 11:16 PM, Anton Yartsev wrote:

> Thanks for the review!
>> 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?
> The goal of this 'if' is to detect an AltiVec vector initialization with a single literal and I know that it should be exactly CStyleCastExpr:
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> .....
> +      if (PE->getNumExprs() == 1) {
> .....
> +        return BuildCStyleCastExpr(LParenLoc, TInfo, RParenLoc, Literal);
> +      }

That's not actually an answer to my question, although perhaps I didn't formulate the question well :)

In C++, should I be able to write a functional-style cast to an AltiVec vector with a single literal? Given that C++ considers functional-style and C-style casts to be equivalent when there's only one argument, I think that we should.

Should static_cast also work? For that, it might depend on what GCC does.

>> 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.
> corrected!
> 
> Can I commit the patch?

I'd prefer that CodeGen just check for a CastExpr with kind CK_VectorSplat; CodeGen isn't supposed to consider syntax. With that tweak, please go ahead.

	- Doug