This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by fpetrogalli on Nov 6 2022, 5:16 PM.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2022, 5:16 PM
fpetrogalli requested review of this revision.Nov 6 2022, 5:16 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 6 2022, 5:16 PM

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

Thanks! I think it's a great improvement!

llvm/lib/Target/RISCV/RISCV.td
568–588

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 added inline comments.Nov 6 2022, 7:07 PM
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 updated this revision to Diff 473545.EditedNov 6 2022, 7:25 PM

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

fpetrogalli marked 2 inline comments as done.Nov 6 2022, 7:26 PM
tmatheson added inline comments.
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 added inline comments.Nov 7 2022, 10:04 AM
llvm/lib/Target/RISCV/RISCV.td
568–588

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").

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 added inline comments.Nov 7 2022, 11:15 AM
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 added a comment.EditedNov 7 2022, 7:25 PM

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 added inline comments.Nov 9 2022, 11:37 PM
llvm/lib/Target/RISCV/RISCV.td
568–589

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?

578

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

khchen added a subscriber: khchen.Nov 10 2022, 10:02 AM

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 marked 2 inline comments as done.Nov 11 2022, 11:28 AM

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.

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.

fpetrogalli retitled this revision from [TargetSupport] Generate the defs for RISCV CPUs using llvm-tblgen. to [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen..Dec 20 2022, 8:11 AM
craig.topper added inline comments.Dec 20 2022, 9:04 AM
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
592–593

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

592–593

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 added a subscriber: lenary.Dec 20 2022, 10:57 AM

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:

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 added inline comments.Dec 20 2022, 11:08 AM
llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt
19 ↗(On Diff #484283)

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

fpetrogalli added inline comments.Dec 20 2022, 11:11 AM
llvm/lib/Target/AArch64/AsmParser/CMakeLists.txt
19 ↗(On Diff #484283)

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

lenary added inline comments.Dec 20 2022, 11:51 AM
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 marked 8 inline comments as done.
  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.

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

@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 marked an inline comment as done.Dec 20 2022, 3:00 PM
fpetrogalli added inline comments.
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 added inline comments.
llvm/include/llvm/module.modulemap
426–427

@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 added inline comments.Dec 20 2022, 3:29 PM
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.

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.

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!

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

fpetrogalli marked 2 inline comments as done.Dec 21 2022, 6:34 AM

Same NFC - restore an empty line change...

barannikov88 added inline comments.
llvm/lib/TargetParser/CMakeLists.txt
27

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 marked 5 inline comments as done.Dec 21 2022, 12:42 PM

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

fpetrogalli added inline comments.Dec 21 2022, 12:44 PM
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 added inline comments.Dec 21 2022, 1:02 PM
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 marked an inline comment as done.Dec 21 2022, 1:03 PM
fpetrogalli added inline comments.
llvm/lib/TargetParser/CMakeLists.txt
27

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 marked an inline comment as done.Dec 21 2022, 1:03 PM
fpetrogalli marked an inline comment as done.Dec 21 2022, 1:07 PM
fpetrogalli added inline comments.
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 marked an inline comment as done.

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.

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 marked an inline comment as done.

Remove RISCVTargetParser.def from the module map.

Follow the guidelines on using namespace llvm.

barannikov88 added inline comments.Dec 21 2022, 1:25 PM
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.

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 marked an inline comment as done.

Remove all auto types in the emitter.

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp
53

I am not a fan either of auto. Removed.

fpetrogalli marked an inline comment as done.Dec 21 2022, 1:43 PM

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

@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, 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

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

@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

fpetrogalli edited the summary of this revision. (Show Details)Dec 22 2022, 4:32 AM

@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 edited the summary of this revision. (Show Details)Dec 22 2022, 4:34 AM

NFC - just an update on top of current main.

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'

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

Thank you,

Francesco

craig.topper added inline comments.Jan 3 2023, 10:00 AM
llvm/lib/Target/RISCV/RISCV.td
572

80 columns

631

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.

llvm/lib/Target/RISCV/RISCV.td
568–588

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

576

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?

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 marked 5 inline comments as done.Jan 4 2023, 3:09 AM

[NFC] Restore empty line before end of namespace.

This revision is now accepted and ready to land.Jan 10 2023, 9:35 AM
jrtc27 added inline comments.Jan 10 2023, 11:16 AM
llvm/lib/Target/RISCV/RISCV.td
581

Formatting changes in RISCV.td (NFC).

fpetrogalli marked an inline comment as done.Jan 10 2023, 1:11 PM

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 added a subscriber: thakis.Jan 11 2023, 9:10 AM
thakis added inline comments.
llvm/include/llvm/TargetParser/CMakeLists.txt
4

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 marked an inline comment as done.Jan 11 2023, 2:33 PM
fpetrogalli added inline comments.
llvm/include/llvm/TargetParser/CMakeLists.txt
4

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 added inline comments.Jan 11 2023, 2:35 PM
llvm/include/llvm/TargetParser/CMakeLists.txt
4

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

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 marked an inline comment as done.Jan 11 2023, 2:39 PM

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.

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.

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?

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 marked an inline comment as done.Jan 12 2023, 3:05 AM
fpetrogalli added inline comments.
llvm/include/llvm/TargetParser/CMakeLists.txt
4

@thakis - is D141581 a less unusual solution?

llvm/include/llvm/TargetParser/RISCVTargetParser.h