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. > > + 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
