[PATCH] D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

Thu Mar 19 04:17:20 PDT 2020

stuij marked an inline comment as done.
stuij added inline comments.

================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:184
+  // Also include the Armv8.5 defines
+  // FIXME: Armv8.6 makes some extensions mandatory. Handle them here.
+  getTargetDefinesARMV85A(Opts, Builder);
----------------
SjoerdMeijer wrote:
> Can you be more specific, what are we missing here?
> 
> Hmm, now I see the same above: "FIXME: Armv8.5 makes some extensions mandatory. Handle them here."
> While you're at it, can you also change that?
Those comments were somewhat of a drive-by nature, made by a GCC contributor. I went through the v8.x extensions and tried to match them with both GCC and LLVM feature defines. Neither LLVM or GCC has defines for all extensions, and after talking to the contributer (Kyrylo Tkachov), it turns out he had at most parity with GCC in mind. 

As an aside GCC seems to have about 14 feature defines that are not present in LLVM, and one feature is spelled __ARM_FEATURE_FP16_FML in GCC and __ARM_FEATURE_FP16FML in LLVM.

After surveying the list, I found a few defines that could be placed here. I've added them as inline comments. I think they deserve their own patch, as this involves multiple revisions and you'd like to make sure that it's all handled sensibly. I believe we have plans to overhaul this general area.

I've removed the fixme's in revisions that didn't actually need them.

================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:52
+              AArch64::AEK_RDM  | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
+              AArch64::AEK_SM4  | AArch64::AEK_SHA3 | AArch64::AEK_BF16    |
+              AArch64::AEK_SHA2 | AArch64::AEK_AES  | AArch64::AEK_I8MM))
----------------
SjoerdMeijer wrote:
> just double checking (because I can't remember): BF16 is a mandatory extension?
for 8.2 it isn't, for 8.6 it is

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76062/new/

https://reviews.llvm.org/D76062