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

[Eli Friedman]

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.

-  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.

+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>.

+  if (Ty->getAs<VectorType>() && (Size == 8 || Size >= 16)) {

Size >= 8?

+    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.

-Eli