The initial patch was updated and separated to two patches (attached). The first patch will fix passing legal vector types as varargs: We make sure the vector is correctly aligned before casting it to the vector type.
The second patch will fix passing illegal vector types as varargs: We will legalize them in clang. Thanks, Manman
arm-abi-vector1.patch
Description: Binary data
arm-abi-vector-all.patch
Description: Binary data
On Oct 11, 2012, at 3:48 PM, manman ren <[email protected]> wrote: > > Thanks for the comments. > > Manman > > On Oct 11, 2012, at 2:17 PM, Eli Friedman <[email protected]> wrote: > >> On Thu, Oct 11, 2012 at 10:20 AM, Manman Ren <[email protected]> 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. <4 xi1> is illegal, we treat vector types with size <= 32-bits as illegal. >> >> + 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. Updated to use memcpy. >> >> -Eli >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
