[PATCH] D68050: WIP Make attribute target work better with AArch64

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for putting up the patch! Some comments inline.



================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:108
   return Name == "generic" ||
-         llvm::AArch64::parseCPUArch(Name) != llvm::AArch64::ArchKind::INVALID;
+         llvm::AArch64::parseArch(Name) != llvm::AArch64::ArchKind::INVALID;
+}
----------------
This does not seem right. The AArch64 target makes the distinction between CPU and architecture names. Unfortunately the documentation for parseCPUArch is non-existent, but it checks if the passed in CPU name matches a valid CPU name and returns the corresponding architecture of the CPU.

. I think with the change, `isValidCPUName("cortex-a57")` will return false, whereas it should return true. `isValidCPUName("armv8-a")` will return true instead of false. 

I think it would be better to use parseArch directly to validate the "arch=" attribute (or add an isValidArchName here).

Also, CPU features/extensions can be validate with llvm::AArch64::parseArchExt().


================
Comment at: clang/test/CodeGen/aarch64-target-attr.c:1
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s | FileCheck %s
+__attribute__((target("arch=armv8-a")))
----------------
Can we also add a few negative tests, including.  `__attribute__((target("arch=armv8.4-a-lse")))`, an invalid architecture name and an invalid CPU feature?


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

https://reviews.llvm.org/D68050