[PATCH] D134353: [AArch64] Add all predecessor archs in target info

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 26 00:55:35 PDT 2022


dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

I think this looks OK. It is a good sign that I had something similar, even if the code came out quite differently.

There are a number of nitpicks below, the general idea LGTM. Thanks for the patch.



================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:533
+  Features[Name] = Enabled;
+  llvm::AArch64::ArchKind AK = llvm::AArch64::getSubArchArchKind(Name);
+  if ("9" == getArchVersionString(AK))
----------------
Add a comment explaining that we add 8.x architectures based on 9.x, and another that we add all previous architectures.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:535
+  if ("9" == getArchVersionString(AK))
+    for (llvm::AArch64::ArchKind i = llvm::AArch64::convertV9toV8(AK);
+         i != llvm::AArch64::ArchKind::INVALID; --i)
----------------
Capitalize `i`. Here and below.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:635
       HasUnaligned = false;
-    if (Feature == "+v8a")
+    // All predeccor archs are added but select the latest one for ArchKind.
+    if (Feature == "+v8a" && ArchKind < llvm::AArch64::ArchKind::ARMV8A)
----------------
predeccor -> predecessor?


================
Comment at: clang/test/CodeGen/aarch64-subarch-compatbility.c:4
+
+// Successor targets shall able to call predecessor target functions.
+__attribute__((__always_inline__,target("v8a")))
----------------
shall able -> should be able?


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:1579
+    if (AK == AArch64::ArchKind::INVALID)
+      EXPECT_TRUE(AK == AArch64::convertV9toV8(AK));
+    else if (AK < AArch64::ArchKind::ARMV9A)
----------------
Use EXPECT_EQ for these?


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:1585
+    else
+      EXPECT_TRUE(AArch64::convertV9toV8(AK) <= AArch64::ArchKind::ARMV9A);
+  }
----------------
Should `<=` be `<`?


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:1587-1588
+  }
+  EXPECT_TRUE(AArch64::ArchKind::ARMV8_5A ==
+              AArch64::convertV9toV8(AArch64::ArchKind::ARMV9A));
+}
----------------
It may be better to list the possibilities for current v9 architectures.


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

https://reviews.llvm.org/D134353



More information about the llvm-commits mailing list