[PATCH] D66018: [ARM] Take into account -mcpu and -mfpu options while handling 'crypto' feature

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio added a comment.

Hi @krisb,
thanks for looking into this, and sorry for the delay, was out for a week.



================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:486-490
+  if (ArchKind == llvm::ARM::ArchKind::ARMV8A ||
+      ArchKind == llvm::ARM::ArchKind::ARMV8_1A ||
+      ArchKind == llvm::ARM::ArchKind::ARMV8_2A ||
+      ArchKind == llvm::ARM::ArchKind::ARMV8_3A ||
+      ArchKind == llvm::ARM::ArchKind::ARMV8_4A) {
----------------
Could we do something a little more permanent here, for example it is already missing ARMV8_5A.
Digging I found the function `StringRef arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple)`.
Perhaps just testing  `(llvm::ARM::parseArchVersion(arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple)) >= 8)`
would do a better job.



================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:659
   llvm::ARM::ArchKind ArchKind;
-  if (CPU == "generic") {
+  if (CPU == "generic" || CPU.empty()) {
     std::string ARMArch = tools::arm::getARMArch(Arch, Triple);
----------------
Good catch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66018