[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