[llvm-commits] [PATCH] ARM ABI: handle varargs with vector types.

[manman ren]

Thanks for the comments.

Manman

On Oct 11, 2012, at 2:17 PM, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Thu, Oct 11, 2012 at 10:20 AM, Manman Ren <mren at apple.com> wrote:
>> 
>> We used to generate incorrect results when passing vector types as varargs.
>> 
>> This patch is intended to fix the issues:
>>  For illegal vector types, we legalize them in clang.
>>  For varargs, make sure the vector is correctly aligned before casting it to the vector type.
> 
> Please separate these out into separate patches.
> 
> I'm not sure your solution for illegal vectors is appropriate; if the
> LLVM calling convention code can't do something sane for <3 x i8> on
> ARM, it's a bug there.
The main reason I legalize the vectors in clang is for varargs. Since we expand varargs in clang and the call site is handled in the back end, it is hard to match exactly how illegal vectors are handled in the backend.

> 
> -  if (!isAggregateTypeForABI(Ty)) {
> +  if (!isAggregateTypeForABI(Ty) && !isIllegalVectorType(Ty)) {
>     // Treat an enum type as its underlying type.
>     if (const EnumType *EnumTy = Ty->getAs<EnumType>())
>       Ty = EnumTy->getDecl()->getIntegerType();
> @@ -2919,6 +2920,27 @@
>             ABIArgInfo::getExtend() : ABIArgInfo::getDirect());
>   }
> 
> +  // Handle illegal vector types here.
> +  if (isIllegalVectorType(Ty)) {
> 
> Weird code structure; just put the isIllegalVectorType check first.
Will do.
> 
> +bool ARMABIInfo::isIllegalVectorType(QualType Ty) const {
> +  if (const VectorType *VT = Ty->getAs<VectorType>()) {
> +    // Check whether VT is legal.
> +    unsigned NumElements = VT->getNumElements();
> +    // NumElements should be power of 2.
> +    if ((NumElements & (NumElements - 1)) != 0)
> +      return true;
> +    return false;
> +  }
> 
> I'm not sure this actually catches all cases of illegal vector
> types... e.g. <4 x i1>.
Will check.
> 
> +  if (Ty->getAs<VectorType>() && (Size == 8 || Size >= 16)) {
> 
> Size >= 8?
Will do.
> 
> +    for (unsigned WordIdx = 0; WordIdx < Size / 4; WordIdx++) {
> +      llvm::Value *Src = Builder.CreateLoad(
> +        Builder.CreateGEP(AddrCast,
> +                          llvm::ConstantInt::get(CGF.Int32Ty, WordIdx),
> +                          "var.src"));
> +      Builder.CreateStore(Src, Builder.CreateGEP(TmpCast,
> +        llvm::ConstantInt::get(CGF.Int32Ty, WordIdx), "var.dst"));
> +    }
> 
> Please use memcpy instead of explicitly expanding it.
Will Check.
> 
> -Eli