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

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added a comment.

Can't approve because I don't know the motivation, but in general this is a good thing to add. I wish the target parser had a better way to model it so we didn't have to rely on the ordering of enum values but that's out of scope for now.



================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:533
+  if ("9" == getArchVersionString(AK)) {
+    unsigned AK_v85 = static_cast<unsigned>(llvm::AArch64::ArchKind::ARMV8_5A);
+    AK_v85 += static_cast<unsigned>(AK) -
----------------
I'd prefer that this sort of logic went in the target parser somewhere. Ok it's yet another one off function but I'd like to keep all these target query functions all in one place.

NormaliseV8V9? Horrible name but you get the idea.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:634
       HasUnaligned = false;
-    if (Feature == "+v8a")
+    if (Feature == "+v8a" && ArchKind < llvm::AArch64::ArchKind::ARMV8A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8A;
----------------
Can you add a comment here explaining the intent of this?

I think it is something like, if the current architecture kind is not equal to what the feature would give you, make the current arch kind equal to the feature's architecture. So if ArchKind was v8.2 and I put in v8.4, we set ArchKind to 8.4. If it was 8.5 to start with, don't change it.

It's not so obvious on first read though.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:116
 
+inline ArchKind operator--(ArchKind Kind) {
+  if (Kind == ArchKind::INVALID)
----------------
We need to at least add comments to the ArchKind definition in target parser to say that order matters now. They are in order but only because it made sense to list them that way.

Perhaps test the order in the target parser tests but that feels like repeating the same stuff and just another test we need to update. So compromise with a comment.


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

https://reviews.llvm.org/D134353