fpetrogalli added a comment. In D137517#4012449 <https://reviews.llvm.org/D137517#4012449>, @pcwang-thead wrote:
> In D137517#4012307 <https://reviews.llvm.org/D137517#4012307>, @craig.topper > wrote: > >> In D137517#4012298 <https://reviews.llvm.org/D137517#4012298>, @pcwang-thead >> wrote: >> >>> In D137517#4009175 <https://reviews.llvm.org/D137517#4009175>, @fpetrogalli >>> wrote: >>> >>>> @pcwang-thead, I addressed some of your comments. >>>> >>>> The value of `EnumFeatures` is now computed dynamicaly from the >>>> `Features` field of the `Processor` class. >>> >>> Thanks! That sounds great to me! >>> >>>> As for generating `MArch` out of the `Features` field, @craig.topper >>>> pointed me at >>>> https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/11. >>>> From >>>> the reading of it, it seems that the alphabetical order is enough to >>>> build the string that carries `MArch`. Am I missing something? >>> >>> Currently, I think the alphabetical order is OK. If we relax the checking >>> of arch string someday, there is no doubt that we should change the >>> implementation here too. >> >> The currently accepted order isn’t alphabetical. The single letter >> extensions have a specific order. The z extensions are ordered by looking up >> the second letter in the single letter order. If we alphabetize here i don’t >> think it will be accepted by the frontend. > > Oops, my mistake. > > Here is my PoC to generate march from Features: > > diff --git a/llvm/lib/Target/RISCV/RISCV.td b/llvm/lib/Target/RISCV/RISCV.td > index d1d0356179f5..b2520f25bfea 100644 > --- a/llvm/lib/Target/RISCV/RISCV.td > +++ b/llvm/lib/Target/RISCV/RISCV.td > @@ -556,8 +556,8 @@ include "RISCVSchedSyntacoreSCR1.td" > class RISCVProcessorModelPROC<string n, > SchedMachineModel m, > list<SubtargetFeature> f, > - string default_march = "", > - list<SubtargetFeature> tunef = []> : > ProcessorModel<n, m, f, tunef> { > + list<SubtargetFeature> tunef = [], > + string default_march = ""> : > ProcessorModel<n, m, f, tunef> { > string DefaultMarch = default_march; > } > diff --git a/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp > b/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp > index b216e82cef6c..eea31e6ddea8 100644 > --- a/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp > +++ b/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp > @@ -13,17 +13,33 @@ > > #include "TableGenBackends.h" > #include "llvm/TableGen/Record.h" > +#include "llvm/Support/RISCVISAInfo.h" > > using namespace llvm; > > -static std::string getEnumFeatures(const Record &Rec) { > +static int getXLen(const Record &Rec) { > std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features"); > if (find_if(Features, [](const Record *R) { > return R->getName() == "Feature64Bit"; > }) != Features.end()) > - return "FK_64BIT"; > + return 64; > > - return "FK_NONE"; > + return 32; > +} > + > +static std::string getMArch(int XLen, const Record &Rec) { > + std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features"); > + std::vector<std::string> FeatureVector; > + // Convert Features to FeatureVector. > + for (auto *Feature : Features) { > + StringRef FeatureName = Feature->getValueAsString("Name"); > + if (llvm::RISCVISAInfo::isSupportedExtensionFeature(FeatureName)) > + FeatureVector.push_back(std::string("+") + FeatureName.str()); > + } > + auto ISAInfo = llvm::RISCVISAInfo::parseFeatures(XLen, FeatureVector); > + if (!ISAInfo) > + report_fatal_error("Invalid features: "); > + return (*ISAInfo)->toString(); > } > > void llvm::EmitRISCVTargetDef(const RecordKeeper &RK, raw_ostream &OS) { > @@ -39,11 +55,17 @@ void llvm::EmitRISCVTargetDef(const RecordKeeper &RK, > raw_ostream &OS) { > // Iterate on all definition records. > for (const MapTy &Def : Map) { > const Record &Rec = *(Def.second); > - if (Rec.isSubClassOf("RISCVProcessorModelPROC")) > + if (Rec.isSubClassOf("RISCVProcessorModelPROC")) { > + int XLen = getXLen(Rec); > + std::string EnumFeatures = XLen == 64 ? "FK_64BIT" : "FK_NONE"; > + std::string MArch = Rec.getValueAsString("DefaultMarch").str(); > + if (MArch == "") > + MArch = getMArch(XLen, Rec); > OS << "PROC(" << Rec.getName() << ", " > - << "{\"" << Rec.getValueAsString("Name") << "\"}," > - << getEnumFeatures(Rec) << ", " > - << "{\"" << Rec.getValueAsString("DefaultMarch") << "\"})\n"; > + << "{\"" << Rec.getValueAsString("Name") << "\"}," << EnumFeatures > + << ", " > + << "{\"" << MArch << "\"})\n"; > + } > } > OS << "\n#undef PROC\n"; > OS << "\n"; > > The generated file would be like below (the march strings are tedious but I > think that would be OK): > > #ifndef PROC > #define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) > #endif > > PROC(INVALID, {"invalid"}, FK_INVALID, {""}) > PROC(GENERIC_RV32, {"generic-rv32"},FK_NONE, {"rv32i2p0"}) > PROC(GENERIC_RV64, {"generic-rv64"},FK_64BIT, {"rv64i2p0"}) > PROC(ROCKET_RV32, {"rocket-rv32"},FK_NONE, {"rv32i2p0"}) > PROC(ROCKET_RV64, {"rocket-rv64"},FK_64BIT, {"rv64i2p0"}) > PROC(SIFIVE_E20, {"sifive-e20"},FK_NONE, {"rv32i2p0_m2p0_c2p0"}) > PROC(SIFIVE_E21, {"sifive-e21"},FK_NONE, {"rv32i2p0_m2p0_a2p0_c2p0"}) > PROC(SIFIVE_E24, {"sifive-e24"},FK_NONE, {"rv32i2p0_m2p0_a2p0_f2p0_c2p0"}) > PROC(SIFIVE_E31, {"sifive-e31"},FK_NONE, {"rv32i2p0_m2p0_a2p0_c2p0"}) > PROC(SIFIVE_E34, {"sifive-e34"},FK_NONE, {"rv32i2p0_m2p0_a2p0_f2p0_c2p0"}) > PROC(SIFIVE_E76, {"sifive-e76"},FK_NONE, {"rv32i2p0_m2p0_a2p0_f2p0_c2p0"}) > PROC(SIFIVE_S21, {"sifive-s21"},FK_64BIT, {"rv64i2p0_m2p0_a2p0_c2p0"}) > PROC(SIFIVE_S51, {"sifive-s51"},FK_64BIT, {"rv64i2p0_m2p0_a2p0_c2p0"}) > PROC(SIFIVE_S54, {"sifive-s54"},FK_64BIT, > {"rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"}) > PROC(SIFIVE_S76, {"sifive-s76"},FK_64BIT, > {"rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"}) > PROC(SIFIVE_U54, {"sifive-u54"},FK_64BIT, > {"rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"}) > PROC(SIFIVE_U74, {"sifive-u74"},FK_64BIT, > {"rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"}) > PROC(SYNTACORE_SCR1_BASE, {"syntacore-scr1-base"},FK_NONE, > {"rv32i2p0_c2p0"}) > PROC(SYNTACORE_SCR1_MAX, {"syntacore-scr1-max"},FK_NONE, > {"rv32i2p0_m2p0_c2p0"}) > > #undef PROC > > #ifndef TUNE_PROC > #define TUNE_PROC(ENUM, NAME) > #endif > > TUNE_PROC(GENERIC, "generic") > TUNE_PROC(ROCKET, "rocket") > TUNE_PROC(SIFIVE_7, "sifive-7-series") > > #undef TUNE_PROC > > The key point is building `RISCVISAInfo` from feature vectors and using > `RISCVISAInfo::toString()` to construct the march string. If there is no > default march string, then we can generate it from CPU's features. > > This will cause a cyclic dependency: tablengen->TargetParser->tablengen, so I > move `RISCVISAInfo.cpp` back to `Support` component in D140529 > <https://reviews.llvm.org/D140529>. > > Is it OK with you? @craig.topper @pcwang-thead, may I ask you to own these further optimisations of the generative process, and submit a patch for it after the current patch lands? I'd happily review it! The reason I am asking this is because the current patch is mostly dealing with making sure we can build clang/llvm after removing the def file. People are discussing dependencies and modules (for example, last update I did was to make the patch work for modules with `-DLLVM_ENABLE_MODULES=On`), and this is already taking quite a number of comments. There is value in the discussion on how to build march out of the features, I'd rather keep it in a separate submission so that the threads do not get lost among the other comments for this patch. Francesco Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137517/new/ https://reviews.llvm.org/D137517 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
