[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.
Tomas Matheson via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 27 03:51:08 PST 2023
tmatheson added a comment.
This patch has made it considerably harder to understand what is going on in the TargetParser. If you get a chance, please could you add some clarifying comments and tidy-ups. I appreciate that a lot of this is following the lead of the pre-existing TargetParser code, but lets try to improve it as we go.
================
Comment at: clang/include/clang/Basic/TargetInfo.h:1345
+ /// generation and get its dependent options in second argument.
+ virtual bool getFeatureDepOptions(StringRef Feature,
+ std::string &Options) const {
----------------
Is this really useful for any other targets? It seems very specific to AArch64 anf FMV, and at the only 2 call sites you are guaranteed to have an AArch64TargetInfo. Seems like an unnecessary extension to the interface.
================
Comment at: clang/include/clang/Basic/TargetInfo.h:1311
+ /// generation.
+ virtual bool isCodeImpactFeatureName(StringRef Feature) const { return true; }
+
----------------
samtebbs wrote:
> Similarly to the warning string, I think a name like `featureAffectsCodeGen(...)` would be more clear in its use.
I agree that the function name doesn't suggest what the doc comment says this function does. Also, the meaning of the return value (does this affect codegen?) seems unrelated to the other output value (list of dependent options) so I don't understand why this is one function rather than two.
================
Comment at: clang/lib/AST/ASTContext.cpp:13302
+ if (Target->validateCpuSupports(Feature.str()))
+ ResFeats.push_back("?" + Feature.str());
+ return ResFeats;
----------------
Is the meaning of this question mark explained anywhere? Could a more meaningful type be used rather than passing around strings?
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:664
+bool AArch64TargetInfo::getFeatureDepOptions(StringRef Name,
+ std::string &FeatureVec) const {
+ FeatureVec = llvm::StringSwitch<std::string>(Name)
----------------
FeatureVec is not a vector.
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:671
+ .Default("");
+ return FeatureVec != "";
+}
----------------
If I'm reading this right, this is saying that the extension "does not affect codegen" (from the docstring) if the extension's dependent features DEP_FEATURES //happens to be the empty string// (or an invalid extension name is passed in). IIUC, this is not a reasonable way to indicate that it does not affect codegen; an extra macro parameter/struct field would be more appropriate.
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.def:108
// FIXME: This would be nicer were it tablegen
-AARCH64_ARCH_EXT_NAME("invalid", AArch64::AEK_INVALID, {}, {})
-AARCH64_ARCH_EXT_NAME("none", AArch64::AEK_NONE, {}, {})
-AARCH64_ARCH_EXT_NAME("crc", AArch64::AEK_CRC, "+crc", "-crc")
-AARCH64_ARCH_EXT_NAME("lse", AArch64::AEK_LSE, "+lse", "-lse")
-AARCH64_ARCH_EXT_NAME("rdm", AArch64::AEK_RDM, "+rdm", "-rdm")
-AARCH64_ARCH_EXT_NAME("crypto", AArch64::AEK_CRYPTO, "+crypto", "-crypto")
-AARCH64_ARCH_EXT_NAME("sm4", AArch64::AEK_SM4, "+sm4", "-sm4")
-AARCH64_ARCH_EXT_NAME("sha3", AArch64::AEK_SHA3, "+sha3", "-sha3")
-AARCH64_ARCH_EXT_NAME("sha2", AArch64::AEK_SHA2, "+sha2", "-sha2")
-AARCH64_ARCH_EXT_NAME("aes", AArch64::AEK_AES, "+aes", "-aes")
-AARCH64_ARCH_EXT_NAME("dotprod", AArch64::AEK_DOTPROD, "+dotprod", "-dotprod")
-AARCH64_ARCH_EXT_NAME("fp", AArch64::AEK_FP, "+fp-armv8", "-fp-armv8")
-AARCH64_ARCH_EXT_NAME("simd", AArch64::AEK_SIMD, "+neon", "-neon")
-AARCH64_ARCH_EXT_NAME("fp16", AArch64::AEK_FP16, "+fullfp16", "-fullfp16")
-AARCH64_ARCH_EXT_NAME("fp16fml", AArch64::AEK_FP16FML, "+fp16fml", "-fp16fml")
-AARCH64_ARCH_EXT_NAME("profile", AArch64::AEK_PROFILE, "+spe", "-spe")
-AARCH64_ARCH_EXT_NAME("ras", AArch64::AEK_RAS, "+ras", "-ras")
-AARCH64_ARCH_EXT_NAME("rasv2", AArch64::AEK_RASv2, "+rasv2", "-rasv2")
-AARCH64_ARCH_EXT_NAME("sve", AArch64::AEK_SVE, "+sve", "-sve")
-AARCH64_ARCH_EXT_NAME("sve2", AArch64::AEK_SVE2, "+sve2", "-sve2")
-AARCH64_ARCH_EXT_NAME("sve2-aes", AArch64::AEK_SVE2AES, "+sve2-aes", "-sve2-aes")
-AARCH64_ARCH_EXT_NAME("sve2-sm4", AArch64::AEK_SVE2SM4, "+sve2-sm4", "-sve2-sm4")
-AARCH64_ARCH_EXT_NAME("sve2-sha3", AArch64::AEK_SVE2SHA3, "+sve2-sha3", "-sve2-sha3")
-AARCH64_ARCH_EXT_NAME("sve2-bitperm", AArch64::AEK_SVE2BITPERM, "+sve2-bitperm", "-sve2-bitperm")
-AARCH64_ARCH_EXT_NAME("sve2p1", AArch64::AEK_SVE2p1, "+sve2p1", "-sve2p1")
-AARCH64_ARCH_EXT_NAME("b16b16", AArch64::AEK_B16B16, "+b16b16", "-b16b16")
-AARCH64_ARCH_EXT_NAME("rcpc", AArch64::AEK_RCPC, "+rcpc", "-rcpc")
-AARCH64_ARCH_EXT_NAME("rng", AArch64::AEK_RAND, "+rand", "-rand")
-AARCH64_ARCH_EXT_NAME("memtag", AArch64::AEK_MTE, "+mte", "-mte")
-AARCH64_ARCH_EXT_NAME("ssbs", AArch64::AEK_SSBS, "+ssbs", "-ssbs")
-AARCH64_ARCH_EXT_NAME("sb", AArch64::AEK_SB, "+sb", "-sb")
-AARCH64_ARCH_EXT_NAME("predres", AArch64::AEK_PREDRES, "+predres", "-predres")
-AARCH64_ARCH_EXT_NAME("bf16", AArch64::AEK_BF16, "+bf16", "-bf16")
-AARCH64_ARCH_EXT_NAME("i8mm", AArch64::AEK_I8MM, "+i8mm", "-i8mm")
-AARCH64_ARCH_EXT_NAME("f32mm", AArch64::AEK_F32MM, "+f32mm", "-f32mm")
-AARCH64_ARCH_EXT_NAME("f64mm", AArch64::AEK_F64MM, "+f64mm", "-f64mm")
-AARCH64_ARCH_EXT_NAME("tme", AArch64::AEK_TME, "+tme", "-tme")
-AARCH64_ARCH_EXT_NAME("ls64", AArch64::AEK_LS64, "+ls64", "-ls64")
-AARCH64_ARCH_EXT_NAME("brbe", AArch64::AEK_BRBE, "+brbe", "-brbe")
-AARCH64_ARCH_EXT_NAME("pauth", AArch64::AEK_PAUTH, "+pauth", "-pauth")
-AARCH64_ARCH_EXT_NAME("flagm", AArch64::AEK_FLAGM, "+flagm", "-flagm")
-AARCH64_ARCH_EXT_NAME("sme", AArch64::AEK_SME, "+sme", "-sme")
-AARCH64_ARCH_EXT_NAME("sme-f64f64", AArch64::AEK_SMEF64F64, "+sme-f64f64", "-sme-f64f64")
-AARCH64_ARCH_EXT_NAME("sme-i16i64", AArch64::AEK_SMEI16I64, "+sme-i16i64", "-sme-i16i64")
-AARCH64_ARCH_EXT_NAME("sme-f16f16", AArch64::AEK_SMEF16F16, "+sme-f16f16", "-sme-f16f16")
-AARCH64_ARCH_EXT_NAME("sme2", AArch64::AEK_SME2, "+sme2", "-sme2")
-AARCH64_ARCH_EXT_NAME("sme2p1", AArch64::AEK_SME2p1, "+sme2p1", "-sme2p1")
-AARCH64_ARCH_EXT_NAME("hbc", AArch64::AEK_HBC, "+hbc", "-hbc")
-AARCH64_ARCH_EXT_NAME("mops", AArch64::AEK_MOPS, "+mops", "-mops")
-AARCH64_ARCH_EXT_NAME("pmuv3", AArch64::AEK_PERFMON, "+perfmon", "-perfmon")
-AARCH64_ARCH_EXT_NAME("predres2", AArch64::AEK_SPECRES2, "+specres2", "-specres2")
-AARCH64_ARCH_EXT_NAME("cssc", AArch64::AEK_CSSC, "+cssc", "-cssc")
-AARCH64_ARCH_EXT_NAME("rcpc3", AArch64::AEK_RCPC3, "+rcpc3", "-rcpc3")
-AARCH64_ARCH_EXT_NAME("the", AArch64::AEK_THE, "+the", "-the")
-AARCH64_ARCH_EXT_NAME("d128", AArch64::AEK_D128, "+d128", "-d128")
-AARCH64_ARCH_EXT_NAME("lse128", AArch64::AEK_LSE128, "+lse128", "-lse128")
+AARCH64_ARCH_EXT_NAME("invalid", AArch64::AEK_INVALID, {}, {}, MAX, "", 0)
+// "none" feature has the maximum allowed function multi versioning priority
----------------
The reader has to know somehow that MAX will be concatenated into FEAT_MAX etc. Please write FEAT_MAX explicitly to make it easier to see what is going on.
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.def:109
+AARCH64_ARCH_EXT_NAME("invalid", AArch64::AEK_INVALID, {}, {}, MAX, "", 0)
+// "none" feature has the maximum allowed function multi versioning priority
+AARCH64_ARCH_EXT_NAME("none", AArch64::AEK_NONE, {}, {}, MAX, "", 1000)
----------------
It would be better to add a named variable like `FMVMaxPriority` than having `"none"` keep track of the magic value.
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.def:167
+ 440)
+AARCH64_ARCH_EXT_NAME("memtag2", AArch64::AEK_NONE, {}, {}, MEMTAG2, "+mte",
+ 450)
----------------
The extensions defined here used to be the things that could be added to march; now it also includes anything that can be specified in attributes? There should be a comment explaining that and making it clear which ones appear in -march now.
Reusing AArch64::AEK_NONE to indicate that the entries should not appear on the command line is confusing; it should at least it should be documented or renamed.
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.def:221
+AARCH64_ARCH_EXT_NAME("sha1", AArch64::AEK_NONE, {}, {}, SHA1,
+ "+fp-armv8,+neon", 120)
+AARCH64_ARCH_EXT_NAME("pmull", AArch64::AEK_NONE, {}, {}, PMULL,
----------------
Why do some of these have themselves as dependencies e.g. `+sha2` while others don't e.g. `+sha1`? It looks like the ones that don't are not available through march? Ideally they would all be done consistently.
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:27
namespace AArch64 {
+enum CPUFeatures {
+ FEAT_RNG,
----------------
Could you add an explanation of what these are, how they differ from the ArchExtKind enum, the fact that FEAT_MAX is a sentinel, etc? Could ArchExtKind not have been extended, rather than having two enums modelling essentially the same thing?
Also this has to be kept in sync with compiler-rt, which should be documented here.
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:183
+#define AARCH64_ARCH_EXT_NAME(NAME, ID, FEATURE, NEGFEATURE, FMV_ID, \
+ DEP_FEATURES, FMV_PRIORITY) \
{NAME, ID, FEATURE, NEGFEATURE},
----------------
Please document what these new fields represent
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:241
uint64_t getDefaultExtensions(StringRef CPU, ArchKind AK);
+void getFeatureOption(StringRef Name, std::string &Feature);
ArchKind getCPUArchKind(StringRef CPU);
----------------
This could return a std::string. Also please document what it returns, it is not clear what a "feature option" is and what the distinction from a "feature" is.
================
Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:253
bool isX18ReservedByDefault(const Triple &TT);
+uint64_t getCpuSupportsMask(ArrayRef<StringRef> FeatureStrs);
----------------
What is a SupportsMask? This seems related to the CPUFeatures enum, please document what is going on.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127812/new/
https://reviews.llvm.org/D127812
More information about the cfe-commits
mailing list