[TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[fpetrogalli]

Summary

This patch removes the file llvm/include/llvm/TargetParser/RISCVTargetParser.def and replaces it with a tablegen-generated .inc file out of llvm/lib/Target/RISCV/RISCV.td.

The module system has been updated to make sure we can build clang/llvm with -DLLVM_ENABLE_MODULES=On

[fpetrogalli]

I updated the text of the header of the file RISCVTargetDefEmitter.cpp. NFC.

[fpetrogalli]

RFC at https://discourse.llvm.org/t/targetparser-auto-generation-of-riscv-cpu-definitions/66419

[• pcwang-thead]

Thanks! I think it's a great improvement!

llvm/lib/Target/RISCV/RISCV.td

556–568

Can EnumFeatures/DefaultMarch string be inferred from ProcessorModel's SubtargetFeature if not specified and Enum just be the uppercase of the name of ProcessorModel? The implementation could be more complicated but I think it's worthy.

[craig.topper]

llvm/lib/Target/RISCV/CMakeLists.txt

18 ↗

(On Diff #473532)

Should not this only be in the CMakeLists for the TargetSupport library?

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

10

This comment says assembly printer but that’s not what this file is.

[fpetrogalli]

I have made the dependency on RISCVTargetSupportTableGen explicit in the RISCVCodeGen target, and fixed the description of the emitter.

[tmatheson]

llvm/lib/TargetSupport/CMakeLists.txt

2 ↗

(On Diff #473545)

Here RISCVTargetSupportTableGen depends on files in lib/Target/RISCV, meaning the folder structure doesn't reflect the dependency graph and it looks like a circular dependency. It would be cleaner to keep the TargetParser data out of lib/Target in e.g. llvm/lib/TargetSupport/RISCVTargetInfo.td. If it is needed in RISCV.td it could be included from there.

[fpetrogalli]

llvm/lib/Target/RISCV/RISCV.td

556–568

Can EnumFeatures/DefaultMarch string be inferred from ProcessorModel's SubtargetFeature if not specified

Sorry, I don't see how. SubtargetFeature is used in the fields Features and TuneFeatures. I don't see the logic to extract this information from them.

and Enum just be the uppercase of the name of ProcessorModel? The implementation could be more complicated but I think it's worthy.

My preference would be to keep the Enum explicit as it would help using those enums in the code. Also, sometimes the enum cannot be derived from the Name (see for example "sifive-7-series" vs "SIFIVE_7").

[fpetrogalli]

When looping through the records in tablegen, I have added an extra (necessary) condition to make sure that the records we are processing are also derived from the class
ProcessorModel.

[fpetrogalli]

llvm/lib/TargetSupport/CMakeLists.txt

2 ↗

(On Diff #473545)

The generator needs the field Name which is stored in the class ProcessorModel. Moving instances of ProcessorModel out of lib/Target/RISCV/RISCV.td seems quite a major change, not just a matter of including a file. I'll give it a go anyway and report back.

[• pcwang-thead]

A rough implementation just for your reference:

diff --git a/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp b/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp
index 47e11b9a1eab..f91f969c2b17 100644
--- a/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp
+++ b/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp
@@ -14,6 +14,32 @@
 #include "llvm/TableGen/Record.h"
 
 namespace llvm {
+struct CPUInfo {
+  std::string EnumName;
+  std::string MArch;
+  std::string Features;
+};
+
+static CPUInfo getCPUInfoFromFeatures(const Record &Record) {
+  std::string EnumName = Record.getValueAsString("Name").upper();
+  EnumName = EnumName.replace(EnumName.find_first_of('-'), 1, "_");
+  std::string MArch;
+  // FIXME: MArch should be in alphabetical order.
+  std::string Features;
+  for (auto *Feature : Record.getValueAsListOfDefs("Features")) {
+    StringRef FeatureName = Feature->getValueAsString("Name");
+    if (FeatureName == "32bit") {
+      MArch += "rv32i";
+      Features = "FK_NONE";
+    } else if (FeatureName == "64bit") {
+      MArch += "rv64i";
+      Features = "FK_64BIT";
+    } else
+      MArch += FeatureName;
+  }
+  return {EnumName, MArch, Features};
+}
+
 void EmitRISCVTargetDef(RecordKeeper &RK, raw_ostream &OS) {
   const auto &Map = RK.getDefs();
 
@@ -26,11 +52,17 @@ void EmitRISCVTargetDef(RecordKeeper &RK, raw_ostream &OS) {
   for (auto &Def : Map) {
     const auto &Record = Def.second;
     if (Record->isSubClassOf("RISCVProcessorModelPROC") &&
-        Record->isSubClassOf("ProcessorModel"))
+        Record->isSubClassOf("ProcessorModel")) {
       OS << "PROC(" << Record->getValueAsString("Enum") << ", "
          << "{\"" << Record->getValueAsString("Name") << "\"}, "
          << Record->getValueAsString("EnumFeatures") << ", "
          << "{\"" << Record->getValueAsString("DefaultMarch") << "\"})\n";
+      const CPUInfo &Info = getCPUInfoFromFeatures(*Record);
+      OS << "// PROC(" << Info.EnumName << ", "
+         << "{\"" << Record->getValueAsString("Name") << "\"}, "
+         << Info.Features << ", "
+         << "{\"" << Info.MArch << "\"})\n";
+    }
   }
   OS << "\n#undef PROC\n";
   OS << "\n";

The generated header would be like:

#ifndef PROC
#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH)
#endif

PROC(INVALID, {"invalid"}, FK_INVALID, {""})
PROC(GENERIC_RV32, {"generic-rv32"}, FK_NONE, {""})
// PROC(GENERIC_RV32, {"generic-rv32"}, FK_NONE, {"rv32i"})
PROC(GENERIC_RV64, {"generic-rv64"}, FK_64BIT, {""})
// PROC(GENERIC_RV64, {"generic-rv64"}, FK_64BIT, {"rv64i"})
PROC(ROCKET_RV32, {"rocket-rv32"}, FK_NONE, {""})
// PROC(ROCKET_RV32, {"rocket-rv32"}, FK_NONE, {"rv32i"})
PROC(ROCKET_RV64, {"rocket-rv64"}, FK_64BIT, {""})
// PROC(ROCKET_RV64, {"rocket-rv64"}, FK_64BIT, {"rv64i"})
PROC(SIFIVE_E20, {"sifive-e20"}, FK_NONE, {"rv32imc"})
// PROC(SIFIVE_E20, {"sifive-e20"}, FK_NONE, {"rv32imc"})
PROC(SIFIVE_E21, {"sifive-e21"}, FK_NONE, {"rv32imac"})
// PROC(SIFIVE_E21, {"sifive-e21"}, FK_NONE, {"rv32imac"})
PROC(SIFIVE_E24, {"sifive-e24"}, FK_NONE, {"rv32imafc"})
// PROC(SIFIVE_E24, {"sifive-e24"}, FK_NONE, {"rv32imafc"})
PROC(SIFIVE_E31, {"sifive-e31"}, FK_NONE, {"rv32imac"})
// PROC(SIFIVE_E31, {"sifive-e31"}, FK_NONE, {"rv32imac"})
PROC(SIFIVE_E34, {"sifive-e34"}, FK_NONE, {"rv32imafc"})
// PROC(SIFIVE_E34, {"sifive-e34"}, FK_NONE, {"rv32imafc"})
PROC(SIFIVE_E76, {"sifive-e76"}, FK_NONE, {"rv32imafc"})
// PROC(SIFIVE_E76, {"sifive-e76"}, FK_NONE, {"rv32imafc"})
PROC(SIFIVE_S21, {"sifive-s21"}, FK_64BIT, {"rv64imac"})
// PROC(SIFIVE_S21, {"sifive-s21"}, FK_64BIT, {"rv64imac"})
PROC(SIFIVE_S51, {"sifive-s51"}, FK_64BIT, {"rv64imac"})
// PROC(SIFIVE_S51, {"sifive-s51"}, FK_64BIT, {"rv64imac"})
PROC(SIFIVE_S54, {"sifive-s54"}, FK_64BIT, {"rv64gc"})
// PROC(SIFIVE_S54, {"sifive-s54"}, FK_64BIT, {"rv64imafdc"})
PROC(SIFIVE_S76, {"sifive-s76"}, FK_64BIT, {"rv64gc"})
// PROC(SIFIVE_S76, {"sifive-s76"}, FK_64BIT, {"rv64imafdc"})
PROC(SIFIVE_U54, {"sifive-u54"}, FK_64BIT, {"rv64gc"})
// PROC(SIFIVE_U54, {"sifive-u54"}, FK_64BIT, {"rv64imafdc"})
PROC(SIFIVE_U74, {"sifive-u74"}, FK_64BIT, {"rv64gc"})
// PROC(SIFIVE_U74, {"sifive-u74"}, FK_64BIT, {"rv64imafdc"})

#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

---

My preference would be to keep the Enum explicit as it would help using those enums in the code. Also, sometimes the enum cannot be derived from the Name (see for example "sifive-7-series" vs "SIFIVE_7").

I believe that we can change it to "SIFIVE-7-SERIES" since this enum name hasn't been used in anywhere and it's just an enum name. :-)

[jrtc27]

llvm/lib/Target/RISCV/RISCV.td

556–569

Given RISCVProcessorModelTUNE_PROC is a subset of RISCVProcessorModelPROC the latter should inherit from the former. Also since every use is also inheriting from ProcessorModel why not make RISCVProcessorModelTUNE_PROC itself inherit from ProcessorModel?

566

This would be more natural in TableGen as def GENERIC_RV32 : ... and dropping the Enum field.

[fpetrogalli]

I have applied the chaged suggested by @jrtc27:

1. The classes RISCVProcessorModelPROC and RISCVProcessorModelTUNE_PROC now are derived from ProcessorModel;
2. The Enum field is removed from both classes. What was read from Enum is now read from the NAME of the tablegen record.

I also updated some of the CmakeLists.txt as the header of llvm/include/llvm/TargetSupport/TargetParser.h was missing the include of the generated file.

[fpetrogalli]

I intend to base this patch on D137838 once it goes through the major refactoring of bridging the header files. For the sake of simplicity, for now I'll keep it on top of D137516.

[fpetrogalli]

This update is not based anymore on D137516, but uses the refactoring of TargetParser as part of LLVMSupport
into the new component LLVMTargetPArser introduced in D137838. NFCI.

[craig.topper]

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

Why do we need to touch CMake file that aren't RISC-V?

llvm/lib/Target/RISCV/RISCV.td

572–573

Line this up to the column after the < on the previous line.

572–573

Line this up under Feature32Bit on the previous line.

llvm/utils/TableGen/CMakeLists.txt

64

I think this list might be in alphabetical order except for the placement of CTagsEmitter. Can you move RISCVTargetDefEmitter.cpp into the right place?

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

11

RISCV->RISC-V

[lenary]

One comment on the build changes, I don't have opinions on the RISC-V changes.

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

Yeah, this shouldn't be needed.

We do have some fixes for the modules build which recently landed, maybe they fix the issues you were seeing, including:

• https://reviews.llvm.org/rG9cd6fbee7ed881f8e80b735e95567040e56f189e
• https://reviews.llvm.org/rG6bdf378dcd349d97152846bb687c1d1de511d138
• https://reviews.llvm.org/D140420 (this isn't landed, but it might clear up some weird things about the quicker modulemap fixes)

[fpetrogalli]

Hi - gentle ping on reviewing this

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

This is unrelated to Modules. The .inc file generated by tablegen is created in {make_build_folder}/lib/TargetParser. The file is then included in TargetParser.cpp but also in TargetParser.h -> this means that every time we include the latter in a cpp file we need to make the inc file visible for inclusion.

[craig.topper]

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

Can we split RISC-V out of TargetParser.cpp and TargetParser.h?

[fpetrogalli]

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

Of course! :) I'll do it in a separate patch.

[lenary]

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

I think, if that is the case, we should add {make_build_folder}/lib/TargetParser as a public include directory of the LLVMTargetParser library, which should mean anything depending on it gets that as an include directory they can rely on.

llvm/lib/Target/RISCV/CMakeLists.txt

69 ↗

(On Diff #484283)

Why is this needed? I already added TargetParser to the list of required components, on (new) line 61, is this doing something else?

[fpetrogalli]

1. I have split the RISCV bits of the target parser out of TargetParser.cpp so that none of the non-RISCV components need to include

the folder with the autogenerated .inc file.

2. I have removed the hard-coded RISCVTargetParser.def
3. Minor cosmetic changes requested from review.

[fpetrogalli]

@pcwang-thead, I addressed some of your comments.

The value of EnumFeatures is now computed dynamicaly from the
Features field of the Processor class.

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?

[fpetrogalli]

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

I think, if that is the case, we should add {make_build_folder}/lib/TargetParser as a public include directory of the LLVMTargetParser library, which should mean anything depending on it gets that as an include directory they can rely on.

How do I do that?I tried target_include_directories(LLVMTargetParser PUBLIC ${LLVM_LIBRARY_DIR}/TargetParser/) but I got:

CMake Error in lib/TargetParser/CMakeLists.txt:
  Target "LLVMTargetParser" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:

    "/Users/fpetrogalli/<...>/build-shared/./lib/TargetParser/"

  which is prefixed in the build directory.

llvm/lib/Target/RISCV/CMakeLists.txt

69 ↗

(On Diff #484283)

My bad - removed!

[fpetrogalli]

llvm/include/llvm/module.modulemap

420 ↗

(On Diff #484391)

@aprantl - I have removed a def file that was needed to build part of the interface of the TargetParser in LLVMSupport. I suspect this might have consequences on the Modules? The correspondent file is now created in the build folder as lib/TargetParser/RISCVTargetParserDef.inc.

Just giving you heads up in case any of the builders that use modules start failing once this code lands in main.

[lenary]

llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt

19 ↗

(On Diff #484283)

I think https://stackoverflow.com/a/25681179 points to how to do this in such a way that it works properly for people using cmake install.

[jrtc27]

Hm, this means that llvm/lib/Target/$TARGET/$TARGET.td needs to remain working (or at least mostly working, things like ISel patterns can fail to type check) in order for Clang to build, since we're now introducing a dependency on llvm/lib/Target/$TARGET in Clang where there didn't used to be (regardless of whether you care about that target). That's probably fine but worth pointing out, especially in the context of experimental targets that are supposed to be mostly ignorable.

[fpetrogalli]

There is no need to use target_include_directories(LLVMRISCVDesc PRIVATE ${LLVM_LIBRARY_DIR}/TargetParser/) for components that are not related to the RISC-V target. Thank you @lenary for the suggestion!

[fpetrogalli]

NFC - I just restored an empty line to prevent modifying a file that is not being touched by the patch.

[fpetrogalli]

Same NFC - restore an empty line change...

[barannikov88]

llvm/lib/TargetParser/CMakeLists.txt

29

Will it work if RISC-V target is not compiled-in?
This does not strictly add a cyclic dependency, but it would still be nice to avoid dependency on higher-level components.
Is it possible / reasonable to extract the part of the RISCV.td that relates to this component and put it separate td file in this directory? Or is it tightly coupled with the rest of the target description?

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

16

<iostream> is forbidden by the coding standards.

17

This should be using namespace llvm;

19

This should be static.

21

(suggestion) LLVM's version of find_if accepts ranges, which is a bit shorter.

29

This function does not seem to mutate RecordKeeper, so it should be const.

38

Should also const, same for the loop below.

53

Same for the loop above.

[fpetrogalli]

I submitted some of the cleanup suggested by @barannikov88 - thank you!

[fpetrogalli]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

16

Ops - that was a leftover for my nasty debugging session :)

17

Hum, if I do this, I get:

Undefined symbols for architecture arm64:
  "llvm::EmitRISCVTargetDef(llvm::RecordKeeper&, llvm::raw_ostream&)", referenced from:
      (anonymous namespace)::LLVMTableGenMain(llvm::raw_ostream&, llvm::RecordKeeper&) in TableGen.cpp.o
ld: symbol(s) not found for architecture arm64

It is a bit surprising because the linking command has utils/TableGen/CMakeFiles/llvm-tblgen.dir/RISCVTargetDefEmitter.cpp.o into it... Some of the files in this folder do not use the convention you pointed at, it it OK if I live it as it is?

29

Done, however none of the other "emitters" in TableGenBackends.h mark the record keeper as const...

[barannikov88]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

17

Right, after using namespace llvm you have to write
llvm::EmitRISCVTargetDef with explicit llvm:: qualification. This is the whole point of this guideline :)
Please see the link.

[fpetrogalli]

llvm/lib/TargetParser/CMakeLists.txt

29

Will it work if RISC-V target is not compiled-in?

This line worked, so yes

cmake ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -GNinja -DLLVM_TARGETS_TO_BUILD="AArch64;AMDGPU;ARM;AVR;BPF;Hexagon;Lanai;Mips;MSP430;NVPTX;PowerPC;Sparc;SystemZ;VE;WebAssembly;X86;XCore"

This does not strictly add a cyclic dependency, but it would still be nice to avoid dependency on higher-level components.
Is it possible / reasonable to extract the part of the RISCV.td that relates to this component and put it separate td file in this directory? Or is it tightly coupled with the rest of the target description?

Hum - the content of RISCV.td is central in llvm/lib/Target/RISCV/. The only way I can see we can put a td file in this folder is by duplicating the content that we use from RISCV.td... That however would mean missing the point of this patch though, as the idea is to have centralised unique source of information.

[fpetrogalli]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

53

/Users/fpetrogalli/projects/cpu-defs/upstream/llvm-project/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:38:17: error: variable 'Record' with type 'const auto *' has incompatible initializer of type 'const std::unique_ptr<llvm::Record>'
    const auto *Record = Def.second;

[fpetrogalli]

ops - this was wrong:

/Users/fpetrogalli/projects/cpu-defs/upstream/llvm-project/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:38:17: error: variable 'Record' with type 'const auto *' has incompatible initializer of type 'const std::unique_ptr<llvm::Record>'
    const auto *Record = Def.second;

I'll revert it.

[fpetrogalli]

Restore the following...

-    const auto *Record = Def.second;
+    const auto &Record = Def.second;

... to prevent this error:

/<...>/llvm-project/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:38:17: error: variable 'Record' with type 'const auto *' has incompatible initializer of type 'const std::unique_ptr<llvm::Record>'
    const auto *Record = Def.second;

[fpetrogalli]

Remove RISCVTargetParser.def from the module map.

Follow the guidelines on using namespace llvm.

[barannikov88]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

53

Whoops, my bad, sorry.
Never liked these 'auto's... Never know what is behind them. Looks like a raw pointer, but it is not.

[fpetrogalli]

In D137517#4009328, @jrtc27 wrote:

Hm, this means that llvm/lib/Target/$TARGET/$TARGET.td needs to remain working (or at least mostly working, things like ISel patterns can fail to type check) in order for Clang to build, since we're now introducing a dependency on llvm/lib/Target/$TARGET in Clang where there didn't used to be (regardless of whether you care about that target). That's probably fine but worth pointing out, especially in the context of experimental targets that are supposed to be mostly ignorable.

Yes,llvm/lib/Target/$TARGET/$TARGET.td need to remain working. But this is true only for those targets that have an auto generative process of the list of CPUs and features needed by the target parser. Right now the only target having this is RISCV. Any target do not need to worry about the dependencies introduced by this patch , until they introduce their own custom generator.

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

17

Yeah, I did that, I just missed adding the declaration via header file inclusion...

@@ -12,8 +12,8 @@
 //===----------------------------------------------------------------------===//

 #include "llvm/TableGen/Record.h"
-
-namespace llvm {
+#include "TableGenBackends.h"
+using namespace llvm;

 static std::string getEnumFeatures(Record &Rec) {
   std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features");
@@ -25,7 +25,7 @@ static std::string getEnumFeatures(Record &Rec) {
   return "FK_NONE";
 }

-void EmitRISCVTargetDef(const RecordKeeper &RK, raw_ostream &OS) {
+void llvm::EmitRISCVTargetDef(const RecordKeeper &RK, raw_ostream &OS) {

[fpetrogalli]

Remove all auto types in the emitter.

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

53

I am not a fan either of auto. Removed.

[• pcwang-thead]

In 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.

[craig.topper]

In D137517#4012298, @pcwang-thead wrote:

In 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.

[• pcwang-thead]

In D137517#4012307, @craig.topper wrote:

In D137517#4012298, @pcwang-thead wrote:

In 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.

Is it OK with you? @craig.topper

[fpetrogalli]

I added some fixes to make it work when building with modules (-DLLVM_ENABLE_MODULES=On).

[fpetrogalli]

In D137517#4012449, @pcwang-thead wrote:

In D137517#4012307, @craig.topper wrote:

In D137517#4012298, @pcwang-thead wrote:

In 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.

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

[• pcwang-thead]

@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

Yes, I am happy to do it. :-)

[fpetrogalli]

NFC - just an update on top of current main.

[fpetrogalli]

Fix pre-merge check at https://buildkite.com/llvm-project/premerge-checks/builds/128468#0185724f-5ade-4603-99ce-f417efb546e8 (missing header inclusion):

C:\ws\w3\llvm-project\premerge-checks\llvm\include\llvm/TargetParser/RISCVTargetParser.h(44): error C2039: 'vector': is not a member of 'std'
C:\BuildTools\VC\Tools\MSVC\14.29.30133\include\string(24): note: see declaration of 'std'
C:\ws\w3\llvm-project\premerge-checks\llvm\include\llvm/TargetParser/RISCVTargetParser.h(44): error C2061: syntax error: identifier 'vector'

[fpetrogalli]

@craig.topper @lenary @barannikov88 @pcwang-thead - New Year's gentle ping :)

Thank you,

Francesco

[craig.topper]

llvm/lib/Target/RISCV/RISCV.td

560

80 columns

611

The formatting is inconsisent. Sometimes the "rv32imafc" is on the end of the previous line. Can we be consistent?

llvm/lib/TargetParser/RISCVTargetParser.cpp

2

This comment should match the name of the file.

[• pcwang-thead]

llvm/lib/Target/RISCV/RISCV.td

556–568

RISCVProcessorModelPROC->RISCVProcessorModel?
RISCVProcessorModelTUNE_PROC->RISCVTuneProcessorModel?
I think it is a little weird that we mixed naming styles here.

564

As for ProcessorModels for tuning, list<SubtargetFeature> f is always empty in both upstream and our downstream, and it is unlikely that we will specify target features for tune models. So it can be a default argument with value [] and swap the position of f and tunef(we are more likely to specify tune features). It becomes:

class RISCVTuneProcessorModel<string n, SchedMachineModel m, list<SubtargetFeature> tunef = [],
    list<SubtargetFeature> f = []> : ProcessorModel<n,m,f,tunef>;

@craig.topper Any thoughts on this?

[fpetrogalli]

I have addressed last round of review from @craig.topper and @pcwang-thead:

1. Renamed the RISCV-specific classes for the ProcessorModel.
2. Swapped tunef with f in RISCVTuneProcessorModel.
3. Fixed comment.
4. Aimed for consistency in formatting of TD files.

[fpetrogalli]

[NFC] Restore empty line before end of namespace.

[fpetrogalli]

Gentle ping!

[craig.topper]

LGTM

[jrtc27]

llvm/lib/Target/RISCV/RISCV.td

569

[fpetrogalli]

Formatting changes in RISCV.td (NFC).

[fpetrogalli]

After submitting this, I had to revert it because of failures like https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio

[fpetrogalli]

In D137517#4042758, @fpetrogalli wrote:

After submitting this, I had to revert it because of failures like https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio

I have resubmitted with what I hope is the right fix (I could not reproduce any of the failures I was seeing in buildbot, on my machine the build is fine).

The new commit is at https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf - in the commit message I have explained what I have changed WRT this original patch. I have added the
tablegen target RISCVTargetParserTableGen in the DEPENDS list of clangDriver and clangBasic. This makes sure that the .*inc file with theist o the CPU is available even if LLVMTargetParser has not been built yet.

[thakis]

llvm/include/llvm/TargetParser/CMakeLists.txt

3 ↗

(On Diff #488109)

All other target tablegen'ing for all other targets (also for RISCV for other .td changes) happen in llvm/lib/Target, not in llvm/TargetParser. Is there any way this could be structured that way too? As-is, the layering looks pretty unusual. (And your reland had to teach clang about tablegen targets for that reason.)

[fpetrogalli]

llvm/include/llvm/TargetParser/CMakeLists.txt

3 ↗

(On Diff #488109)

Thanks for the feedback.

I can think of two possible solutions:

1. Move the RISCV-only part of llvm/lib/TargetParser into llvm/lib/Target
2. Move the whole TargetParser inside Target

However, I am not really sure if these are less unusual than the current state of things (or even technically possible).

I am open for suggestions, and even more open to review a solution that fixes the unusual layering ;).

[craig.topper]

llvm/include/llvm/TargetParser/CMakeLists.txt

3 ↗

(On Diff #488109)

I could be wrong, but I don't think clang depends on the LLVM's Target libraries today.

[jrtc27]

In D137517#4042875, @fpetrogalli wrote:

In D137517#4042758, @fpetrogalli wrote:

After submitting this, I had to revert it because of failures like https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio

I have resubmitted with what I hope is the right fix (I could not reproduce any of the failures I was seeing in buildbot, on my machine the build is fine).

The new commit is at https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf - in the commit message I have explained what I have changed WRT this original patch. I have added the
tablegen target RISCVTargetParserTableGen in the DEPENDS list of clangDriver and clangBasic. This makes sure that the .*inc file with theist o the CPU is available even if LLVMTargetParser has not been built yet.

But you didn't use the proper Differential Revision tag, so the diff here didn't get updated to reflect the amended version committed :(

[fpetrogalli]

In D137517#4045299, @jrtc27 wrote:

In D137517#4042875, @fpetrogalli wrote:

In D137517#4042758, @fpetrogalli wrote:

After submitting this, I had to revert it because of failures like https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio

I have resubmitted with what I hope is the right fix (I could not reproduce any of the failures I was seeing in buildbot, on my machine the build is fine).

The new commit is at https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf - in the commit message I have explained what I have changed WRT this original patch. I have added the
tablegen target RISCVTargetParserTableGen in the DEPENDS list of clangDriver and clangBasic. This makes sure that the .*inc file with theist o the CPU is available even if LLVMTargetParser has not been built yet.

But you didn't use the proper Differential Revision tag, so the diff here didn't get updated to reflect the amended version committed :(

What should I have done? Add the Differential Revision: https://reviews.llvm.org/D137517 as the last line of the commit with the rework? I wasn't aware of this for reworks.

[jrtc27]

In D137517#4045315, @fpetrogalli wrote:

In D137517#4045299, @jrtc27 wrote:

In D137517#4042875, @fpetrogalli wrote:

In D137517#4042758, @fpetrogalli wrote:

After submitting this, I had to revert it because of failures like https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio

I have resubmitted with what I hope is the right fix (I could not reproduce any of the failures I was seeing in buildbot, on my machine the build is fine).

The new commit is at https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf - in the commit message I have explained what I have changed WRT this original patch. I have added the
tablegen target RISCVTargetParserTableGen in the DEPENDS list of clangDriver and clangBasic. This makes sure that the .*inc file with theist o the CPU is available even if LLVMTargetParser has not been built yet.

But you didn't use the proper Differential Revision tag, so the diff here didn't get updated to reflect the amended version committed :(

What should I have done? Add the Differential Revision: https://reviews.llvm.org/D137517 as the last line of the commit with the rework? I wasn't aware of this for reworks.

Yes, it should have the same trailer as the original commit, otherwise it won't be correctly tracked by Phabricator. A "rework" isn't special, it's revert, reopen the revision, update the revision and land the revision again. If re-review isn't needed then you can skip some of the middle, but that's it. Though in this case I do think re-review was warranted, the new clang dependency seems a bit dubious and hints at the design not being quite right.

[fpetrogalli]

In D137517#4045337, @jrtc27 wrote:

In D137517#4045315, @fpetrogalli wrote:

In D137517#4045299, @jrtc27 wrote:

In D137517#4042875, @fpetrogalli wrote:

In D137517#4042758, @fpetrogalli wrote:

After submitting this, I had to revert it because of failures like https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio

I have resubmitted with what I hope is the right fix (I could not reproduce any of the failures I was seeing in buildbot, on my machine the build is fine).

The new commit is at https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf - in the commit message I have explained what I have changed WRT this original patch. I have added the
tablegen target RISCVTargetParserTableGen in the DEPENDS list of clangDriver and clangBasic. This makes sure that the .*inc file with theist o the CPU is available even if LLVMTargetParser has not been built yet.

But you didn't use the proper Differential Revision tag, so the diff here didn't get updated to reflect the amended version committed :(

What should I have done? Add the Differential Revision: https://reviews.llvm.org/D137517 as the last line of the commit with the rework? I wasn't aware of this for reworks.

Yes, it should have the same trailer as the original commit, otherwise it won't be correctly tracked by Phabricator. A "rework" isn't special, it's revert, reopen the revision, update the revision and land the revision again. If re-review isn't needed then you can skip some of the middle, but that's it. Though in this case I do think re-review was warranted, the new clang dependency seems a bit dubious and hints at the design not being quite right.

My bad - I assumed that adding such tablegenning dependency in clang was a minor thing. I'll see if I can remove it. Any suggestions on how this could be done without duplicating the information in RISCV.td?

[sebastian-ne]

FYI, the CMake file should use PROJECT_SOURCE_DIR instead of CMAKE_SOURCE_DIR, otherwise it breaks builds that use CMake’s add_subdirectory. I put up D141521 to fix that.

[fpetrogalli]

llvm/include/llvm/TargetParser/CMakeLists.txt

3 ↗

(On Diff #488109)

@thakis - is D141581 a less unusual solution?

clang/lib/Basic/Targets/RISCV.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCV.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/MacroBuilder.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "llvm/ADT/StringSwitch.h"
-#include "llvm/Support/TargetParser.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/TargetParser/RISCVTargetParser.h"
 #include <optional>
 
 using namespace clang;
 using namespace clang::targets;
 
 ArrayRef<const char *> RISCVTargetInfo::getGCCRegNames() const {
   static const char *const GCCRegNames[] = {
       // Integer registers

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

 #include "clang/Basic/CharInfo.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/RISCVISAInfo.h"
-#include "llvm/Support/TargetParser.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/TargetParser/RISCVTargetParser.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 // Returns false if an error is diagnosed.
 static bool getArchFeatures(const Driver &D, StringRef Arch,

llvm/include/llvm/TargetParser/RISCVTargetParser.h

+//===-- RISCVTargetParser - Parser for target features ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements a target parser to recognise hardware features
+// FOR RISC-V CPUS.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TARGETPARSER_RISCVTARGETPARSER_H
+#define LLVM_TARGETPARSER_RISCVTARGETPARSER_H
+
+#include "llvm/ADT/StringRef.h"
+
+namespace llvm {
+namespace RISCV {
+
+// We use 64 bits as the known part in the scalable vector types.
+static constexpr unsigned RVVBitsPerBlock = 64;
+
+enum CPUKind : unsigned {
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) CK_##ENUM,
+#define TUNE_PROC(ENUM, NAME) CK_##ENUM,
+#include "RISCVTargetParserDef.inc"
+};
+
+enum FeatureKind : unsigned {
+  FK_INVALID = 0,
+  FK_NONE = 1,
+  FK_64BIT = 1 << 2,
+};
+
+bool checkCPUKind(CPUKind Kind, bool IsRV64);
+bool checkTuneCPUKind(CPUKind Kind, bool IsRV64);
+CPUKind parseCPUKind(StringRef CPU);
+CPUKind parseTuneCPUKind(StringRef CPU, bool IsRV64);
+StringRef getMArchFromMcpu(StringRef CPU);
+void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64);
+void fillValidTuneCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64);
+bool getCPUFeaturesExceptStdExt(CPUKind Kind, std::vector<StringRef> &Features);
+
+} // namespace RISCV
+} // namespace llvm
+
+#endif

llvm/include/llvm/TargetParser/RISCVTargetParser.def

-#ifndef PROC
-#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH)
-#endif
-
-PROC(INVALID, {"invalid"}, FK_INVALID, {""})
-PROC(GENERIC_RV32, {"generic-rv32"}, FK_NONE, {""})
-PROC(GENERIC_RV64, {"generic-rv64"}, FK_64BIT, {""})
-PROC(ROCKET_RV32, {"rocket-rv32"}, FK_NONE, {""})
-PROC(ROCKET_RV64, {"rocket-rv64"}, FK_64BIT, {""})
-PROC(SIFIVE_E20, {"sifive-e20"}, FK_NONE, {"rv32imc"})
-PROC(SIFIVE_E21, {"sifive-e21"}, FK_NONE, {"rv32imac"})
-PROC(SIFIVE_E24, {"sifive-e24"}, FK_NONE, {"rv32imafc"})
-PROC(SIFIVE_E31, {"sifive-e31"}, FK_NONE, {"rv32imac"})
-PROC(SIFIVE_E34, {"sifive-e34"}, FK_NONE, {"rv32imafc"})
-PROC(SIFIVE_E76, {"sifive-e76"}, FK_NONE, {"rv32imafc"})
-PROC(SIFIVE_S21, {"sifive-s21"}, FK_64BIT, {"rv64imac"})
-PROC(SIFIVE_S51, {"sifive-s51"}, FK_64BIT, {"rv64imac"})
-PROC(SIFIVE_S54, {"sifive-s54"}, FK_64BIT, {"rv64gc"})
-PROC(SIFIVE_S76, {"sifive-s76"}, FK_64BIT, {"rv64gc"})
-PROC(SIFIVE_U54, {"sifive-u54"}, FK_64BIT, {"rv64gc"})
-PROC(SIFIVE_U74, {"sifive-u74"}, FK_64BIT, {"rv64gc"})
-PROC(SYNTACORE_SCR1_BASE, {"syntacore-scr1-base"}, FK_NONE, {"rv32ic"})
-PROC(SYNTACORE_SCR1_MAX, {"syntacore-scr1-max"}, FK_NONE, {"rv32imc"})
-
-#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

llvm/include/llvm/TargetParser/TargetParser.h

 unsigned getArchAttrR600(GPUKind AK);
 
 void fillValidArchListAMDGCN(SmallVectorImpl<StringRef> &Values);
 void fillValidArchListR600(SmallVectorImpl<StringRef> &Values);
 
 IsaVersion getIsaVersion(StringRef GPU);
 
 } // namespace AMDGPU
-
-namespace RISCV {
-
-// We use 64 bits as the known part in the scalable vector types.
-static constexpr unsigned RVVBitsPerBlock = 64;
-
-enum CPUKind : unsigned {
-#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) CK_##ENUM,
-#define TUNE_PROC(ENUM, NAME) CK_##ENUM,
-#include "RISCVTargetParser.def"
-};
-
-enum FeatureKind : unsigned {
-  FK_INVALID = 0,
-  FK_NONE = 1,
-  FK_64BIT = 1 << 2,
-};
-
-bool checkCPUKind(CPUKind Kind, bool IsRV64);
-bool checkTuneCPUKind(CPUKind Kind, bool IsRV64);
-CPUKind parseCPUKind(StringRef CPU);
-CPUKind parseTuneCPUKind(StringRef CPU, bool IsRV64);
-StringRef getMArchFromMcpu(StringRef CPU);
-void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64);
-void fillValidTuneCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64);
-bool getCPUFeaturesExceptStdExt(CPUKind Kind, std::vector<StringRef> &Features);
-
-} // namespace RISCV
 } // namespace llvm
 
 #endif

llvm/lib/Target/RISCV/RISCV.td

 include "RISCVSchedRocket.td"
 include "RISCVSchedSiFive7.td"
 include "RISCVSchedSyntacoreSCR1.td"
 
 //===----------------------------------------------------------------------===//
 // RISC-V processors supported.
 //===----------------------------------------------------------------------===//
 
-def : ProcessorModel<"generic-rv32", NoSchedModel, [Feature32Bit]>;
-def : ProcessorModel<"generic-rv64", NoSchedModel, [Feature64Bit]>;
+class RISCVProcessorModelPROC<string n,
+                              SchedMachineModel m,
+                              list<SubtargetFeature> f,
+                              string default_march = "",
+                              list<SubtargetFeature> tunef = []> :  ProcessorModel<n, m, f, tunef> {
+  string DefaultMarch = default_march;
+}
+
+class RISCVProcessorModelTUNE_PROC<string n, SchedMachineModel m, list<SubtargetFeature> f,
+                      list<SubtargetFeature> tunef = []> : ProcessorModel<n,m,f,tunef>;
+
+def GENERIC_RV32 : RISCVProcessorModelPROC<"generic-rv32", NoSchedModel, [Feature32Bit]>;
+def GENERIC_RV64 : RISCVProcessorModelPROC<"generic-rv64", NoSchedModel, [Feature64Bit]>;
 // Support generic for compatibility with other targets. The triple will be used
 // to change to the appropriate rv32/rv64 version.
 def : ProcessorModel<"generic", NoSchedModel, []>;
 
-def : ProcessorModel<"rocket-rv32", RocketModel, [Feature32Bit]>;
-def : ProcessorModel<"rocket-rv64", RocketModel, [Feature64Bit]>;
-def : ProcessorModel<"rocket", RocketModel, []>;
+def ROCKET_RV32 : RISCVProcessorModelPROC<"rocket-rv32", RocketModel, [Feature32Bit]>;
+def ROCKET_RV64 : RISCVProcessorModelPROC<"rocket-rv64", RocketModel, [Feature64Bit]>;
+def ROCKET : RISCVProcessorModelTUNE_PROC<"rocket", RocketModel, []>;
 
-def : ProcessorModel<"sifive-7-series", SiFive7Model, [],
+def SIFIVE_7 : RISCVProcessorModelTUNE_PROC<"sifive-7-series", SiFive7Model, [],
                                              [TuneSiFive7]>;
 
-def : ProcessorModel<"sifive-e20", RocketModel, [Feature32Bit,
+def SIFIVE_E20 : RISCVProcessorModelPROC<"sifive-e20", RocketModel, [Feature32Bit,
                                                                      FeatureStdExtM,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv32imc">;
 
-def : ProcessorModel<"sifive-e21", RocketModel, [Feature32Bit,
+def SIFIVE_E21 : RISCVProcessorModelPROC<"sifive-e21", RocketModel, [Feature32Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv32imac">;
 
-def : ProcessorModel<"sifive-e24", RocketModel, [Feature32Bit,
+def SIFIVE_E24 : RISCVProcessorModelPROC<"sifive-e24", RocketModel, [Feature32Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
                                                                      FeatureStdExtF,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv32imafc">;
 
-def : ProcessorModel<"sifive-e31", RocketModel, [Feature32Bit,
+def SIFIVE_E31 : RISCVProcessorModelPROC<"sifive-e31", RocketModel, [Feature32Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv32imac">;
 
-def : ProcessorModel<"sifive-e34", RocketModel, [Feature32Bit,
+def SIFIVE_E34 : RISCVProcessorModelPROC<"sifive-e34", RocketModel, [Feature32Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
                                                                      FeatureStdExtF,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv32imafc">;
 
-def : ProcessorModel<"sifive-e76", SiFive7Model, [Feature32Bit,
+def SIFIVE_E76 : RISCVProcessorModelPROC<"sifive-e76", SiFive7Model, [Feature32Bit,
                                                                       FeatureStdExtM,
                                                                       FeatureStdExtA,
                                                                       FeatureStdExtF,
                                                                       FeatureStdExtC],
-                     [TuneSiFive7]>;
+                                         "rv32imafc", [TuneSiFive7]>;
 
-def : ProcessorModel<"sifive-s21", RocketModel, [Feature64Bit,
+def SIFIVE_S21 : RISCVProcessorModelPROC<"sifive-s21", RocketModel, [Feature64Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv64imac">;
 
-def : ProcessorModel<"sifive-s51", RocketModel, [Feature64Bit,
+def SIFIVE_S51 : RISCVProcessorModelPROC<"sifive-s51", RocketModel, [Feature64Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv64imac">;
 
-def : ProcessorModel<"sifive-s54", RocketModel, [Feature64Bit,
+def SIFIVE_S54 : RISCVProcessorModelPROC<"sifive-s54", RocketModel, [Feature64Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
                                                                      FeatureStdExtF,
                                                                      FeatureStdExtD,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv64gc">;
 
-def : ProcessorModel<"sifive-s76", SiFive7Model, [Feature64Bit,
+def SIFIVE_S76 : RISCVProcessorModelPROC<"sifive-s76", SiFive7Model, [Feature64Bit,
                                                                       FeatureStdExtM,
                                                                       FeatureStdExtA,
                                                                       FeatureStdExtF,
                                                                       FeatureStdExtD,
                                                                       FeatureStdExtC],
-                     [TuneSiFive7]>;
+                                         "rv64gc", [TuneSiFive7]>;
 
-def : ProcessorModel<"sifive-u54", RocketModel, [Feature64Bit,
+def SIFIVE_U54 : RISCVProcessorModelPROC<"sifive-u54", RocketModel, [Feature64Bit,
                                                                      FeatureStdExtM,
                                                                      FeatureStdExtA,
                                                                      FeatureStdExtF,
                                                                      FeatureStdExtD,
-                                                 FeatureStdExtC]>;
+                                                                     FeatureStdExtC], "rv64gc">;
 
-def : ProcessorModel<"sifive-u74", SiFive7Model, [Feature64Bit,
+def SIFIVE_U74 : RISCVProcessorModelPROC<"sifive-u74", SiFive7Model, [Feature64Bit,
                                                                       FeatureStdExtM,
                                                                       FeatureStdExtA,
                                                                       FeatureStdExtF,
                                                                       FeatureStdExtD,
                                                                       FeatureStdExtC],
-                     [TuneSiFive7]>;
+                                         "rv64gc",[TuneSiFive7] >;
 
-def : ProcessorModel<"syntacore-scr1-base", SyntacoreSCR1Model,
+def SYNTACORE_SCR1_BASE : RISCVProcessorModelPROC<"syntacore-scr1-base", SyntacoreSCR1Model,
                                                   [Feature32Bit, FeatureStdExtC],
-                     [TuneNoDefaultUnroll]>;
+                                                  "rv32ic", [TuneNoDefaultUnroll]>;
 
-def : ProcessorModel<"syntacore-scr1-max", SyntacoreSCR1Model,
+def SYNTACORE_SCR1_MAX : RISCVProcessorModelPROC<"syntacore-scr1-max", SyntacoreSCR1Model,
                                                  [Feature32Bit, FeatureStdExtM, FeatureStdExtC],
-                     [TuneNoDefaultUnroll]>;
+                                                 "rv32imc", [TuneNoDefaultUnroll]>;
 
 //===----------------------------------------------------------------------===//
 // Define the RISC-V target.
 //===----------------------------------------------------------------------===//
 
 def RISCVInstrInfo : InstrInfo {
   let guessInstructionProperties = 0;
 }

llvm/lib/Target/RISCV/RISCVISelLowering.h

 
 #ifndef LLVM_LIB_TARGET_RISCV_RISCVISELLOWERING_H
 #define LLVM_LIB_TARGET_RISCV_RISCVISELLOWERING_H
 
 #include "RISCV.h"
 #include "llvm/CodeGen/CallingConvLower.h"
 #include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/TargetLowering.h"
-#include "llvm/Support/TargetParser.h"
+#include "llvm/TargetParser/RISCVTargetParser.h"
 #include <optional>
 
 namespace llvm {
 class RISCVSubtarget;
 struct RISCVRegisterInfo;
 namespace RISCVISD {
 enum NodeType : unsigned {
   FIRST_NUMBER = ISD::BUILTIN_OP_END,

llvm/lib/TargetParser/CMakeLists.txt

+set(LLVM_TARGET_DEFINITIONS ${CMAKE_SOURCE_DIR}/lib/Target/RISCV/RISCV.td)
+tablegen(LLVM RISCVTargetParserDef.inc -gen-riscv-target-def -I ${CMAKE_SOURCE_DIR}/lib/Target/RISCV/)
+add_public_tablegen_target(RISCVTargetParserTableGen)
 
 add_llvm_component_library(LLVMTargetParser
   AArch64TargetParser.cpp
   ARMTargetParserCommon.cpp
   ARMTargetParser.cpp
   CSKYTargetParser.cpp
   Host.cpp
   LoongArchTargetParser.cpp
   RISCVISAInfo.cpp
+  RISCVTargetParser.cpp
   TargetParser.cpp
   Triple.cpp
   X86TargetParser.cpp
 
   ADDITIONAL_HEADER_DIRS
   Unix
   Windows
 
   LINK_COMPONENTS
   Support
   )
+
+# This is needed to make sure that the file RISCVTargetParserDef.inc
+# is visible every time we build a target that depend on
+# LLVMTargetParser. See https://stackoverflow.com/a/25681179
+target_include_directories(LLVMTargetParser PUBLIC $<BUILD_INTERFACE:${LLVM_LIBRARY_DIR}/TargetParser/>)

llvm/lib/TargetParser/RISCVTargetParser.cpp

+//===-- TargetParser - Parser for target features ---------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements a target parser to recognise hardware features
+// FOR RISC-V CPUS.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/TargetParser/RISCVTargetParser.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringSwitch.h"
+
+#include <vector>
+namespace llvm {
+namespace RISCV {
+
+struct CPUInfo {
+  StringLiteral Name;
+  CPUKind Kind;
+  unsigned Features;
+  StringLiteral DefaultMarch;
+  bool is64Bit() const { return (Features & FK_64BIT); }
+};
+
+constexpr CPUInfo RISCVCPUInfo[] = {
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH)                              \
+  {NAME, CK_##ENUM, FEATURES, DEFAULT_MARCH},
+#include "RISCVTargetParserDef.inc"
+};
+
+bool checkCPUKind(CPUKind Kind, bool IsRV64) {
+  if (Kind == CK_INVALID)
+    return false;
+  return RISCVCPUInfo[static_cast<unsigned>(Kind)].is64Bit() == IsRV64;
+}
+
+bool checkTuneCPUKind(CPUKind Kind, bool IsRV64) {
+  if (Kind == CK_INVALID)
+    return false;
+#define TUNE_PROC(ENUM, NAME)                                                  \
+  if (Kind == CK_##ENUM)                                                       \
+    return true;
+#include "RISCVTargetParserDef.inc"
+  return RISCVCPUInfo[static_cast<unsigned>(Kind)].is64Bit() == IsRV64;
+}
+
+CPUKind parseCPUKind(StringRef CPU) {
+  return llvm::StringSwitch<CPUKind>(CPU)
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) .Case(NAME, CK_##ENUM)
+#include "RISCVTargetParserDef.inc"
+      .Default(CK_INVALID);
+}
+
+CPUKind parseTuneCPUKind(StringRef TuneCPU, bool IsRV64) {
+  return llvm::StringSwitch<CPUKind>(TuneCPU)
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) .Case(NAME, CK_##ENUM)
+#define TUNE_PROC(ENUM, NAME) .Case(NAME, CK_##ENUM)
+#include "RISCVTargetParserDef.inc"
+      .Default(CK_INVALID);
+}
+
+StringRef getMArchFromMcpu(StringRef CPU) {
+  CPUKind Kind = parseCPUKind(CPU);
+  return RISCVCPUInfo[static_cast<unsigned>(Kind)].DefaultMarch;
+}
+
+void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64) {
+  for (const auto &C : RISCVCPUInfo) {
+    if (C.Kind != CK_INVALID && IsRV64 == C.is64Bit())
+      Values.emplace_back(C.Name);
+  }
+}
+
+void fillValidTuneCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64) {
+  for (const auto &C : RISCVCPUInfo) {
+    if (C.Kind != CK_INVALID && IsRV64 == C.is64Bit())
+      Values.emplace_back(C.Name);
+  }
+#define TUNE_PROC(ENUM, NAME) Values.emplace_back(StringRef(NAME));
+#include "RISCVTargetParserDef.inc"
+}
+
+// Get all features except standard extension feature
+bool getCPUFeaturesExceptStdExt(CPUKind Kind,
+                                std::vector<StringRef> &Features) {
+  unsigned CPUFeatures = RISCVCPUInfo[static_cast<unsigned>(Kind)].Features;
+
+  if (CPUFeatures == FK_INVALID)
+    return false;
+
+  if (CPUFeatures & FK_64BIT)
+    Features.push_back("+64bit");
+  else
+    Features.push_back("-64bit");
+
+  return true;
+}
+
+} // namespace RISCV
+} // namespace llvm

llvm/lib/TargetParser/TargetParser.cpp

 StringRef AMDGPU::getCanonicalArchName(const Triple &T, StringRef Arch) {
   assert(T.isAMDGPU());
   auto ProcKind = T.isAMDGCN() ? parseArchAMDGCN(Arch) : parseArchR600(Arch);
   if (ProcKind == GK_NONE)
     return StringRef();
 
   return T.isAMDGCN() ? getArchNameAMDGCN(ProcKind) : getArchNameR600(ProcKind);
 }
-
-namespace llvm {
-namespace RISCV {
-
-struct CPUInfo {
-  StringLiteral Name;
-  CPUKind Kind;
-  unsigned Features;
-  StringLiteral DefaultMarch;
-  bool is64Bit() const { return (Features & FK_64BIT); }
-};
-
-constexpr CPUInfo RISCVCPUInfo[] = {
-#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH)                              \
-  {NAME, CK_##ENUM, FEATURES, DEFAULT_MARCH},
-#include "llvm/TargetParser/RISCVTargetParser.def"
-};
-
-bool checkCPUKind(CPUKind Kind, bool IsRV64) {
-  if (Kind == CK_INVALID)
-    return false;
-  return RISCVCPUInfo[static_cast<unsigned>(Kind)].is64Bit() == IsRV64;
-}
-
-bool checkTuneCPUKind(CPUKind Kind, bool IsRV64) {
-  if (Kind == CK_INVALID)
-    return false;
-#define TUNE_PROC(ENUM, NAME) if (Kind == CK_##ENUM) return true;
-#include "llvm/TargetParser/RISCVTargetParser.def"
-  return RISCVCPUInfo[static_cast<unsigned>(Kind)].is64Bit() == IsRV64;
-}
-
-CPUKind parseCPUKind(StringRef CPU) {
-  return llvm::StringSwitch<CPUKind>(CPU)
-#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) .Case(NAME, CK_##ENUM)
-#include "llvm/TargetParser/RISCVTargetParser.def"
-      .Default(CK_INVALID);
-}
-
-CPUKind parseTuneCPUKind(StringRef TuneCPU, bool IsRV64) {
-  return llvm::StringSwitch<CPUKind>(TuneCPU)
-#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) .Case(NAME, CK_##ENUM)
-#define TUNE_PROC(ENUM, NAME) .Case(NAME, CK_##ENUM)
-#include "llvm/TargetParser/RISCVTargetParser.def"
-      .Default(CK_INVALID);
-}
-
-StringRef getMArchFromMcpu(StringRef CPU) {
-  CPUKind Kind = parseCPUKind(CPU);
-  return RISCVCPUInfo[static_cast<unsigned>(Kind)].DefaultMarch;
-}
-
-void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64) {
-  for (const auto &C : RISCVCPUInfo) {
-    if (C.Kind != CK_INVALID && IsRV64 == C.is64Bit())
-      Values.emplace_back(C.Name);
-  }
-}
-
-void fillValidTuneCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64) {
-  for (const auto &C : RISCVCPUInfo) {
-    if (C.Kind != CK_INVALID && IsRV64 == C.is64Bit())
-      Values.emplace_back(C.Name);
-  }
-#define TUNE_PROC(ENUM, NAME) Values.emplace_back(StringRef(NAME));
-#include "llvm/TargetParser/RISCVTargetParser.def"
-}
-
-// Get all features except standard extension feature
-bool getCPUFeaturesExceptStdExt(CPUKind Kind,
-                                std::vector<StringRef> &Features) {
-  unsigned CPUFeatures = RISCVCPUInfo[static_cast<unsigned>(Kind)].Features;
-
-  if (CPUFeatures == FK_INVALID)
-    return false;
-
-  if (CPUFeatures & FK_64BIT)
-    Features.push_back("+64bit");
-  else
-    Features.push_back("-64bit");
-
-  return true;
-}
-
-} // namespace RISCV
-} // namespace llvm

llvm/utils/TableGen/CMakeLists.txt

add_tablegen(llvm-tblgen LLVM
   OptEmitter.cpp
   OptParserEmitter.cpp
   OptRSTEmitter.cpp
   PredicateExpander.cpp
   PseudoLoweringEmitter.cpp
   CompressInstEmitter.cpp
   RegisterBankEmitter.cpp
   RegisterInfoEmitter.cpp
+  RISCVTargetDefEmitter.cpp
   SDNodeProperties.cpp
   SearchableTableEmitter.cpp
   SubtargetEmitter.cpp
   SubtargetFeatureInfo.cpp
   TableGen.cpp
   Types.cpp
   VarLenCodeEmitterGen.cpp
   X86DisassemblerTables.cpp
   X86EVEX2VEXTablesEmitter.cpp
   X86FoldTablesEmitter.cpp
   X86MnemonicTables.cpp
   X86ModRMFilters.cpp
   X86RecognizableInstr.cpp
   WebAssemblyDisassemblerEmitter.cpp
   CTagsEmitter.cpp
   )
 target_link_libraries(llvm-tblgen PRIVATE LLVMTableGenGlobalISel)
 set_target_properties(llvm-tblgen PROPERTIES FOLDER "Tablegenning")

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

+//===- RISCVTargetDefEmitter.cpp - Generate lists of RISCV CPUs -----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This tablegen backend emits the include file needed by the target
+// parser to parse the RISC-V CPUs.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/TableGen/Record.h"
+
+namespace llvm {
+
+static std::string getEnumFeatures(Record &Rec) {
+  std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features");
+  if (find_if(Features, [](Record *R) {
+        return R->getName() == "Feature64Bit";
+      }) != Features.end())
+    return "FK_64BIT";
+
+  return "FK_NONE";
+}
+
+void EmitRISCVTargetDef(const RecordKeeper &RK, raw_ostream &OS) {
+  const auto &Map = RK.getDefs();
+
+  OS << "#ifndef PROC\n"
+     << "#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH)\n"
+     << "#endif\n\n";
+
+  OS << "PROC(INVALID, {\"invalid\"}, FK_INVALID, {\"\"})\n";
+  // Iterate on all definition records.
+  for (const auto &Def : Map) {
+    const auto &Record = Def.second;
+    if (Record->isSubClassOf("RISCVProcessorModelPROC"))
+      OS << "PROC(" << Record->getName() << ", "
+         << "{\"" << Record->getValueAsString("Name") << "\"},"
+         << getEnumFeatures(*Record) << ", "
+         << "{\"" << Record->getValueAsString("DefaultMarch") << "\"})\n";
+  }
+  OS << "\n#undef PROC\n";
+  OS << "\n";
+  OS << "#ifndef TUNE_PROC\n"
+     << "#define TUNE_PROC(ENUM, NAME)\n"
+     << "#endif\n\n";
+  OS << "TUNE_PROC(GENERIC, \"generic\")\n";
+  for (const auto &Def : Map) {
+    const auto &Record = Def.second;
+    if (Record->isSubClassOf("RISCVProcessorModelTUNE_PROC"))
+      OS << "TUNE_PROC(" << Record->getName() << ", "
+         << "\"" << Record->getValueAsString("Name") << "\")\n";
+  }
+
+  OS << "\n#undef TUNE_PROC\n";
+}
+
+} // namespace llvm

llvm/utils/TableGen/TableGen.cpp

enum ActionType {
   GenX86FoldTables,
   GenX86MnemonicTables,
   GenRegisterBank,
   GenExegesis,
   GenAutomata,
   GenDirectivesEnumDecl,
   GenDirectivesEnumImpl,
   GenDXILOperation,
+  GenRISCVTargetDef,
 };
 
 namespace llvm {
 cl::opt<bool> EmitLongStrLiterals(
     "long-string-literals",
     cl::desc("when emitting large string tables, prefer string literals over "
              "comma-separated char literals. This can be a readability and "
              "compile-time performance win, but upsets some compilers"),
cl::values(
         clEnumValN(GenExegesis, "gen-exegesis",
                    "Generate llvm-exegesis tables"),
         clEnumValN(GenAutomata, "gen-automata", "Generate generic automata"),
         clEnumValN(GenDirectivesEnumDecl, "gen-directive-decl",
                    "Generate directive related declaration code (header file)"),
         clEnumValN(GenDirectivesEnumImpl, "gen-directive-impl",
                    "Generate directive related implementation code"),
         clEnumValN(GenDXILOperation, "gen-dxil-operation",
-                   "Generate DXIL operation information")));
-
+                   "Generate DXIL operation information"),
+        clEnumValN(GenRISCVTargetDef, "gen-riscv-target-def",
+                   "Generate the list of CPU for RISCV")));
 cl::OptionCategory PrintEnumsCat("Options for -print-enums");
 cl::opt<std::string> Class("class", cl::desc("Print Enum list for this class"),
                            cl::value_desc("class name"),
                            cl::cat(PrintEnumsCat));
 
 bool LLVMTableGenMain(raw_ostream &OS, RecordKeeper &Records) {
   switch (Action) {
   case PrintRecords:
case GenDirectivesEnumDecl:
     EmitDirectivesDecl(Records, OS);
     break;
   case GenDirectivesEnumImpl:
     EmitDirectivesImpl(Records, OS);
     break;
   case GenDXILOperation:
     EmitDXILOperation(Records, OS);
     break;
+  case GenRISCVTargetDef:
+    EmitRISCVTargetDef(Records, OS);
+    break;
   }
 
   return false;
 }
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);

llvm/utils/TableGen/TableGenBackends.h

 void EmitX86FoldTables(RecordKeeper &RK, raw_ostream &OS);
 void EmitX86MnemonicTables(RecordKeeper &RK, raw_ostream &OS);
 void EmitRegisterBank(RecordKeeper &RK, raw_ostream &OS);
 void EmitExegesis(RecordKeeper &RK, raw_ostream &OS);
 void EmitAutomata(RecordKeeper &RK, raw_ostream &OS);
 void EmitDirectivesDecl(RecordKeeper &RK, raw_ostream &OS);
 void EmitDirectivesImpl(RecordKeeper &RK, raw_ostream &OS);
 void EmitDXILOperation(RecordKeeper &RK, raw_ostream &OS);
-
+void EmitRISCVTargetDef(const RecordKeeper &RK, raw_ostream &OS);
 } // End llvm namespace
 
 #endif