[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Implementation LGTM.



================
Comment at: include/clang/Basic/AttrDocs.td:1753
+This means it is more efficient to call such functions from code that performs
+extensive floating-point and vector calculations, because fewer live SIMD&FP
+registers need to be saved. This property makes it well-suited for e.g.
----------------
Please spell out "and".


================
Comment at: include/clang/Basic/AttrDocs.td:1758
+
+Using this attribute however, also means that it is more expensive to call
+a function that adheres to the default calling convention from within such
----------------
"However, using this attribute also means..."


================
Comment at: include/clang/Basic/AttrDocs.td:1766
+
+.. _`aarch64_vector_pcs`: https://developer.arm.com/products/software-development-tools/hpc/arm-compiler-for-hpc/vector-function-abi
+  }];
----------------
Thanks; other than those two editorial comments, this seems good.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:1101
   case CC_AAPCS:
+  case CC_AArch64VectorCall:
     return llvm::dwarf::DW_CC_LLVM_AAPCS;
----------------
sdesmalen wrote:
> rjmccall wrote:
> > rnk wrote:
> > > sdesmalen wrote:
> > > > I wasn't really sure whether this requires a corresponding DW_CC_LLVM_AAVPCS record in LLVM, as I couldn't find much about the DW_CC_LLVM_  encodings, specifically whether they align with some agreed encoding that is implemented by GDB/LLDB. Is this defined anywhere, or is it ignored by debuggers at the moment?
> > > DWARF only allows encoding 256 conventions, and we grabbed 0xC[0-F], I guess for "clang", so we probably want to be careful about adding another. Do you anticipate making debuggers able to call such functions? If not, it's probably not worth it.
> > They probably should be callable.
> > 
> > It looks like DWARF reserves the first 64 conventions for general/language purposes and treats the rest of the range as "user" conventions.   If those conventions are assumed to be universally unique, that's a really limiting schema once you started dividing it up by vendor.  If I might make a suggestion, while there are certainly many calling conventions that are meant to have universal meaning (e.g. most language-specific conventions), there are also a large number that are inherently target-specific.  DWARF already uses a lot of numbers that only make sense in the context of a target (like register numbers); it would make sense for DWARF to carve out a range of the encoding space (maybe 16 or 32 numbers) for target-specific CCs.  This is hardly the first example; consider also all the variant ARM32 CCs or the i386 fastcall CC.
> Great feedback. I think this discussion has a wider scope than this patch and I think its probably best to keep this change as-is for now. We'll first work to add a section to the 'DWARF for the ARM 64-bit Architecture' document describing a DW_AT_calling_convention value for the AArch64 vector PCS and will create a separate patch to LLVM/Clang to implement its support. I've also asked @keith.walker.arm to raise this (encoding space) as a topic with the DWARF standardization committee.
Thank you, I appreciate that.


https://reviews.llvm.org/D54425