[cfe-commits] [PATCH] AArch64 backend: Clang changes

[John McCall]

On Jan 11, 2013, at 3:36 AM, Tim Northover <tim.northover at arm.com> wrote:
> This is the second version of the Clang patch for the AArch64 target. I think I've applied all of the review comments received so far, the ones which applied to Clang were:
> 
> + Improved comments and documentation (thanks Dmitri)
> + No more grep in tests (Dmitri again)
> 
> To my knowledge there have been no detailed reviews yet, so everything should be considered suspect (though there was much less innovation needed on the Clang side; mostly just implementing ABIs).

-    case BuiltinType::SChar:     EltName = "poly8_t"; break;
-    case BuiltinType::Short:     EltName = "poly16_t"; break;
+    case BuiltinType::UChar:     EltName = "poly8_t"; break;
+    case BuiltinType::UShort:     EltName = "poly16_t"; break;

It's not appropriate for this patch to change the element type of these
vectors for existing platforms.  If you really want to do this — you believe
that these vectors should have unsigned char elements even on ARM32 —
then please prepare a separate patch for that change and start a new thread,
because that will require discussion.  Otherwise, if you just want to change
them on AArch64, then you need to make this code conditional on the
target platform.

     assert((E->getObjectKind() == OK_ObjCProperty ||
+            CT == Ctx.getCanonicalType(Ctx.getBuiltinVaListType()) ||
             !Ctx.getLangOpts().CPlusPlus) &&
            "C++ struct assignment should be resolved by the "
            "copy assignment operator.");

This is a fairly suspicious change.  Why are you ending up in this code for
struct __va_list in C++?

+void CXXNameMangler::mangleAArch64NeonVectorType(const VectorType *T) {
+  QualType EltType = T->getElementType();
+  assert(EltType->isBuiltinType() && "Neon vector element not a BuiltinType");
+  const char *EltName = 0;
+  unsigned BitSize = (T->getNumElements() *
+                      getASTContext().getTypeSize(EltType));

T->getNumElements() is getting consistently mangled into your type here.
Why not just make a function to mangle the vector element (returning
"Poly8" or "Uint16" or whatever) and then construct the rest of the mangling
programmatically?

If that function returned an llvm::StringRef, you wouldn't need to do the strlen,
either.

+
+  if (BitSize != 64 && BitSize != 128)
+    assert((BitSize == 64 || BitSize == 128) &&
+      "Neon vector type not 64 or 128 bits");

You've written the assert condition twice.  Just assert.

+  if (T->getVectorKind() == VectorType::NeonPolyVector) {
+    switch (cast<BuiltinType>(EltType)->getKind()) {
+    //FIXME: poly types should be signed or unsigned?

This seems like a good FIXME to resolve right away.

+    if (llvm::Triple::aarch64 ==
+        getASTContext().getTargetInfo().getTriple().getArch())

Prefer to write this the other way around.

Index: lib/CodeGen/CGCall.cpp
===================================================================
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1253,7 +1253,6 @@
 
     case ABIArgInfo::Extend:
     case ABIArgInfo::Direct: {
-
       // If we have the trivial case, handle it with no muss and fuss.
       if (!isa<llvm::StructType>(ArgI.getCoerceToType()) &&
           ArgI.getCoerceToType() == ConvertType(Ty) &&

Spurious whitespace change.

--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp

It seems to be the case that aarch64's ABI is just the ARM32 ABI except
that ABI guards are 64-bit.  A cleaner way of expressing this would be to
say that aarch64 uses the ARM ABI, and that the ARM ABI says that ABI
guards are size_t's.  Are you expecting further divergences?

+  StringRef getARCRetainAutoreleasedReturnValueMarker() const {
+    llvm_unreachable("ARC not supported");
+  }

Just don't implement the function.

+ABIArgInfo
+AArch64ABIInfo::tryUseRegs(QualType Ty, int &FreeRegs, int RegsNeeded,
+                           bool IsInt, llvm::Type *DirectTy) const {
+  if (FreeRegs >= RegsNeeded) {
+    FreeRegs -= RegsNeeded;
+    return ABIArgInfo::getDirect(DirectTy);
+  }
+
+  llvm::Type *Padding = 0;
+
+  // We need padding so that later arguments don't get filled in anyway. That
+  // wouldn't happen if only ByVal arguments followed in the same category, but
+  // a large structure will simply seem to be a pointer as far as LLVM is
+  // concerned.

Can I kibbitz about your ABI here, or is it too late and you've actually
committed to it?  Because some of these decisions seem unfortunate.
It seems very strange that an argument which doesn't fit in registers is "padded"
to *act as if it occupies registers that it doesn't occupy*.  In the APCS this
made some small amount of sense, because it helped established a general
rule that, regardless of the function signature, you could push r0..r3
in the prologue and end up with a traditional everything-on-the-stack
arrangement.  But AAPCS and the AArch64 convention seem to pass
enough things in variant registers that this property is basically dead, and
honestly I'm not sure it was ever particularly valuable.  Why "fill" registers
like this with "padding"?  Doesn't this just pointlessly force things to the
stack and bloat the va_arg code with additional bookkeeping?

I will note that your va_arg code is *huge*.

-  // If this is a 'float' (CVR qualified or typedef) promote to double.
-  if (Ty->isSpecificBuiltinType(BuiltinType::Float))
+  // If this is a 'float' or '__fp16' (CVR qualified or typedef) promote to
+  // double.
+  if (Ty->isSpecificBuiltinType(BuiltinType::Float) ||
+      Ty->isSpecificBuiltinType(BuiltinType::Half))

This should getAs<BuiltinType>() and check the kind instead of effectively
doing the getAs<> twice.

   const BuiltinType* BTy = CurType->getAs<BuiltinType>();
   if (!BTy ||
       (VecKind == VectorType::NeonPolyVector &&
-       BTy->getKind() != BuiltinType::SChar &&
-       BTy->getKind() != BuiltinType::Short) ||
+       BTy->getKind() != BuiltinType::UChar &&
+       BTy->getKind() != BuiltinType::UShort) ||
       (BTy->getKind() != BuiltinType::SChar &&
        BTy->getKind() != BuiltinType::UChar &&
        BTy->getKind() != BuiltinType::Short &&
        BTy->getKind() != BuiltinType::UShort &&
        BTy->getKind() != BuiltinType::Int &&
        BTy->getKind() != BuiltinType::UInt &&
        BTy->getKind() != BuiltinType::LongLong &&
        BTy->getKind() != BuiltinType::ULongLong &&
-       BTy->getKind() != BuiltinType::Float)) {
+       BTy->getKind() != BuiltinType::Float &&
+       BTy->getKind() != BuiltinType::Half &&
+       // double type is supported in AArch64
+       BTy->getKind() != BuiltinType::Double)) {

If vectors of 'double' are only acceptable in AArch64, you need to be
testing for that, not just widening the net for ARM32 as well.

Also, the same question from above about changing the signedness
of the element type of poly vectors applies here.

John.