Page MenuHomePhabricator

[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
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
580

80 columns

639

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 added inline comments.Jan 3 2023, 7:35 PM
llvm/lib/Target/RISCV/RISCV.td
576–596

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

584

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.Tue, Jan 10, 9:35 AM
jrtc27 added inline comments.Tue, Jan 10, 11:16 AM
llvm/lib/Target/RISCV/RISCV.td
589

Formatting changes in RISCV.td (NFC).

fpetrogalli marked an inline comment as done.Tue, Jan 10, 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.Wed, Jan 11, 9:10 AM
thakis added inline comments.
llvm/include/llvm/TargetParser/CMakeLists.txt
3

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.Wed, Jan 11, 2:33 PM
fpetrogalli added inline comments.
llvm/include/llvm/TargetParser/CMakeLists.txt
3

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.Wed, Jan 11, 2:35 PM
llvm/include/llvm/TargetParser/CMakeLists.txt
3

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.Wed, Jan 11, 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.Thu, Jan 12, 3:05 AM
fpetrogalli added inline comments.
llvm/include/llvm/TargetParser/CMakeLists.txt
3

@thakis - is D141581 a less unusual solution?