[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

I can only comment on the target features part of the patch - I've been hoping it would add something similar and looks very useful in its own right.



================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:63
+  switch (ArchKind) {
+  case llvm::AArch64::ArchKind::ARMV8_8A:
+  case llvm::AArch64::ArchKind::ARMV8_7A:
----------------
There can be an 8.9 / 9.4 now.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:1020
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
+  // Add target and fmv features.
+  for (const auto &Feature : FeaturesVec)
----------------
fmv features -> dependant features.
Can you add a comment explaining the two loops? I assume it is because the second could be -, turning off features that have been turned on?


================
Comment at: clang/test/CodeGen/aarch64-targetattr.c:93
 // CHECK: attributes #0 = { {{.*}} "target-features"="+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #1 = { {{.*}} "target-features"="+sve,+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #2 = { {{.*}} "target-features"="+sve2,+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #4 = { {{.*}} "target-features"="+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
-// CHECK: attributes #5 = { {{.*}} "target-cpu"="cortex-a710" "target-features"="+bf16,+crc,+dotprod,+flagm,+fp-armv8,+fp16fml,+i8mm,+lse,+mte,+neon,+pauth,+ras,+rcpc,+rdm,+sb,+sve,+sve2,+sve2-bitperm" }
-// CHECK: attributes #6 = { {{.*}} "tune-cpu"="cortex-a710" }
-// CHECK: attributes #7 = { {{.*}} "target-cpu"="generic" }
-// CHECK: attributes #8 = { {{.*}} "tune-cpu"="generic" }
-// CHECK: attributes #9 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+spe,+ssbs" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #10 = { {{.*}} "target-features"="+sve" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #11 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+bf16,+crc,+crypto,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+lse,+neon,+rand,+ras,+rcpc,+rdm,+spe,+ssbs,+sve,+sve2" }
-// CHECK: attributes #12 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+bf16,+crc,+crypto,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+lse,+neon,+rand,+ras,+rcpc,+rdm,+spe,+ssbs,-sve" }
-// CHECK: attributes #13 = { {{.*}} "target-features"="+sve" }
-// CHECK: attributes #14 = { {{.*}} "target-features"="+sve,-sve2" }
-// CHECK: attributes #15 = { {{.*}} "target-features"="+fullfp16" }
-// CHECK: attributes #16 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+spe,+ssbs,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #17 = { {{.*}} "branch-target-enforcement"="true" {{.*}} "target-cpu"="neoverse-n1" "target-features"="+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+spe,+ssbs,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #18 = { {{.*}} "target-features"="-neon" }
+// CHECK: attributes #1 = { {{.*}} "target-features"="+complxnum,+fp-armv8,+fullfp16,+jsconv,+neon,+sve,+v8.1a,+v8.2a,+v8a" }
+// CHECK: attributes #2 = { {{.*}} "target-features"="+complxnum,+fp-armv8,+fullfp16,+jsconv,+neon,+sve,+sve2,+v8.1a,+v8.2a,+v8a" }
----------------
These updates look good, thanks for the improvements!


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:107
+AARCH64_ARCH_EXT_NAME("invalid",       AArch64::AEK_INVALID,     {},              {},             \
+MAX,          "",                                                                        0)
+AARCH64_ARCH_EXT_NAME("none",          AArch64::AEK_NONE,        {},              {},             \
----------------
This table could maybe be formatted a little more nicely. LLVM's clang format style would usually align the `(` and the next line. Perhaps putting the last integer before the string can help too.
```
AARCH64_ARCH_EXT_NAME("invalid",       AArch64::AEK_INVALID,     {},              {},             \
                      MAX, 0, "")
```
We could alternatively just clang-format the whole thing.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:115
+AARCH64_ARCH_EXT_NAME("rdm",           AArch64::AEK_RDM,         "+rdm",          "-rdm",         \
+RDM,          "+rdm,+fp-armv8,+neon,+jsconv,+complxnum",                                70)
+AARCH64_ARCH_EXT_NAME("crypto",        AArch64::AEK_CRYPTO,      "+crypto",       "-crypto",      \
----------------
Should RDM be dependant on +jsconv,+complxnum?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127812