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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
