[RISCV] Generate march string from target features

[• pcwang-thead]

Summary

As what has been mentioned in D137517, this patch is to simplify
processors definitions in RISCV.td. We don't have to specify march
string since we can generate it from target features.

[fpetrogalli]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

59

I think you should reference here the pieces of the standard that describe the rules for building the March string.

60

Do you need this variable? it is used only once...

67

I think you can avoid constructing a std::string here?

70

Please avoid auto if the type cannot be deducted from the context.

75

Do we need this variable? It is used only once...

76

You could use StringRef and then check MArch.empty() in line 70.

[• pcwang-thead]

Address comments.

[• pcwang-thead]

Reduce constructions of std::string.

[• pcwang-thead]

Fix errors caused by destroyed string.

[• pcwang-thead]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

76

getMArch() returns std::string, if I change type of MArch from std::string to StringRef, it would be:

if (MArch.empty())
        MArch = StringRef(getMArch(XLen, Rec));

The string will be destroyed and cause errors like https://buildkite.com/llvm-project/premerge-checks/builds/130218.
So I think it is no bother to do that.

[fpetrogalli]

LGTM, with some minor observations:

1. You say The disadvantage is that the generated march string is too tedious.. That does seems to imply that autogenerating the string is not a good idea? I'd remove it from the commit message.

2. I am still thinking you should reference the bits of the RISCV specs that describe how to create MArch out of the features.

Thank you!

Francesco

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

68

Nit - maybe if you set std::vector<StringRef> FeatureVector; you can avoid the .str() in here?

71

Nit: maybe alias the type in the body of the function?

using ISAInfoTy = llvm::Expected<std::unique_ptr<RISCVISAInfo>>;
ISAInfoTy ISAInfo = ...

[• pcwang-thead]

• Add reference to RISCV spec.
• Address comments.

[• pcwang-thead]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

68

I'm afraid not, because we expect std::vector<std::string> in llvm::RISCVISAInfo::parseFeatures.

[fpetrogalli]

llvm/utils/TableGen/RISCVTargetDefEmitter.cpp

33

Nit: link to the specific version, or mention the version.

68

Oh - OK - thanks

[kito-cheng]

LGTM, Ilike this improvement, it's further simplified the step of adding a new CPU info.

[• pcwang-thead]

• Rebase.
• Add spec version.

llvm/lib/Target/RISCV/RISCV.td

 
 //===----------------------------------------------------------------------===//
 // RISC-V processors supported.
 //===----------------------------------------------------------------------===//
 
 class RISCVProcessorModel<string n,
                           SchedMachineModel m,
                           list<SubtargetFeature> f,
-                          string default_march = "",
-                          list<SubtargetFeature> tunef = []>
+                          list<SubtargetFeature> tunef = [],
+                          string default_march = "">
       :  ProcessorModel<n, m, f, tunef> {
   string DefaultMarch = default_march;
 }
 
 class RISCVTuneProcessorModel<string n,
                               SchedMachineModel m,
                               list<SubtargetFeature> tunef = [],
                               list<SubtargetFeature> f = []>

 def SIFIVE_7 : RISCVTuneProcessorModel<"sifive-7-series",
                                        SiFive7Model,
                                        [TuneSiFive7]>;
 
 def SIFIVE_E20 : RISCVProcessorModel<"sifive-e20",
                                      RocketModel,
                                      [Feature32Bit,
                                       FeatureStdExtM,
-                                      FeatureStdExtC],
-                                     "rv32imc">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_E21 : RISCVProcessorModel<"sifive-e21",
                                      RocketModel,
                                      [Feature32Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
-                                      FeatureStdExtC],
-                                     "rv32imac">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_E24 : RISCVProcessorModel<"sifive-e24",
                                      RocketModel,
                                      [Feature32Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
                                       FeatureStdExtF,
-                                      FeatureStdExtC],
-                                     "rv32imafc">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_E31 : RISCVProcessorModel<"sifive-e31",
                                      RocketModel,
                                      [Feature32Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
-                                      FeatureStdExtC],
-                                     "rv32imac">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_E34 : RISCVProcessorModel<"sifive-e34",
                                      RocketModel,
                                      [Feature32Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
                                       FeatureStdExtF,
-                                      FeatureStdExtC],
-                                     "rv32imafc">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_E76 : RISCVProcessorModel<"sifive-e76",
                                      SiFive7Model,
                                      [Feature32Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
                                       FeatureStdExtF,
                                       FeatureStdExtC],
-                                     "rv32imafc",
                                      [TuneSiFive7]>;
 
 def SIFIVE_S21 : RISCVProcessorModel<"sifive-s21",
                                      RocketModel,
                                      [Feature64Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
-                                      FeatureStdExtC],
-                                     "rv64imac">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_S51 : RISCVProcessorModel<"sifive-s51",
                                      RocketModel,
                                      [Feature64Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
-                                      FeatureStdExtC],
-                                     "rv64imac">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_S54 : RISCVProcessorModel<"sifive-s54",
                                       RocketModel,
                                       [Feature64Bit,
                                        FeatureStdExtM,
                                        FeatureStdExtA,
                                        FeatureStdExtF,
                                        FeatureStdExtD,
-                                       FeatureStdExtC],
-                                      "rv64gc">;
+                                       FeatureStdExtC]>;
 
 def SIFIVE_S76 : RISCVProcessorModel<"sifive-s76",
                                      SiFive7Model,
                                      [Feature64Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
                                       FeatureStdExtF,
                                       FeatureStdExtD,
                                       FeatureStdExtC],
-                                     "rv64gc",
                                      [TuneSiFive7]>;
 
 def SIFIVE_U54 : RISCVProcessorModel<"sifive-u54",
                                      RocketModel,
                                      [Feature64Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
                                       FeatureStdExtF,
                                       FeatureStdExtD,
-                                      FeatureStdExtC],
-                                     "rv64gc">;
+                                      FeatureStdExtC]>;
 
 def SIFIVE_U74 : RISCVProcessorModel<"sifive-u74",
                                      SiFive7Model,
                                      [Feature64Bit,
                                       FeatureStdExtM,
                                       FeatureStdExtA,
                                       FeatureStdExtF,
                                       FeatureStdExtD,
                                       FeatureStdExtC],
-                                     "rv64gc",
                                      [TuneSiFive7] >;
 
 def SYNTACORE_SCR1_BASE : RISCVProcessorModel<"syntacore-scr1-base",
                                               SyntacoreSCR1Model,
                                               [Feature32Bit,
                                                FeatureStdExtC],
-                                              "rv32ic",
                                               [TuneNoDefaultUnroll]>;
 
 def SYNTACORE_SCR1_MAX : RISCVProcessorModel<"syntacore-scr1-max",
                                              SyntacoreSCR1Model,
                                              [Feature32Bit,
                                               FeatureStdExtM,
                                               FeatureStdExtC],
-                                             "rv32imc",
                                              [TuneNoDefaultUnroll]>;
 
 //===----------------------------------------------------------------------===//
 // Define the RISC-V target.
 //===----------------------------------------------------------------------===//
 
 def RISCVInstrInfo : InstrInfo {
   let guessInstructionProperties = 0;

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 "TableGenBackends.h"
+#include "llvm/Support/RISCVISAInfo.h"
 #include "llvm/TableGen/Record.h"
 
 using namespace llvm;
 
-static std::string getEnumFeatures(const Record &Rec) {
+using ISAInfoTy = llvm::Expected<std::unique_ptr<RISCVISAInfo>>;
+
+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 32;
+}
+
+// We can generate march string from target features as what has been described
+// in RISCV ISA specification 'Chapter 27. ISA Extension Naming Conventions'.
+//
+// This is almost the same as RISCVFeatures::parseFeatureBits, except that we
+// get feature name from feature records instead of feature bits.
+static std::string getMArch(int XLen, const Record &Rec) {
+  std::vector<std::string> FeatureVector;
+
+  // Convert features to FeatureVector.
+  for (auto *Feature : Rec.getValueAsListOfDefs("Features")) {
+    StringRef FeatureName = Feature->getValueAsString("Name");
+    if (llvm::RISCVISAInfo::isSupportedExtensionFeature(FeatureName))
+      FeatureVector.push_back((Twine("+") + FeatureName).str());
+  }
+
+  ISAInfoTy ISAInfo = llvm::RISCVISAInfo::parseFeatures(XLen, FeatureVector);
+  if (!ISAInfo)
+    report_fatal_error("Invalid features");
+
+  // RISCVISAInfo::toString will generate a march string with all the extensions
+  // we have added to it.
+  return (*ISAInfo)->toString();
+}
 
-  return "FK_NONE";
+static std::string getEnumFeatures(int XLen) {
+  return XLen == 64 ? "FK_64BIT" : "FK_NONE";
 }
 
 void llvm::EmitRISCVTargetDef(const RecordKeeper &RK, raw_ostream &OS) {
   using MapTy = std::pair<const std::string, std::unique_ptr<llvm::Record>>;
   using RecordMap = std::map<std::string, std::unique_ptr<Record>, std::less<>>;
   const RecordMap &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 MapTy &Def : Map) {
     const Record &Rec = *(Def.second);
-    if (Rec.isSubClassOf("RISCVProcessorModel"))
+    if (Rec.isSubClassOf("RISCVProcessorModel")) {
+      int XLen = getXLen(Rec);
+      std::string MArch = Rec.getValueAsString("DefaultMarch").str();
+
+      // Compute MArch from features if we don't specify it.
+      if (MArch.empty())
+        MArch = getMArch(XLen, Rec);
+
       OS << "PROC(" << Rec.getName() << ", "
          << "{\"" << Rec.getValueAsString("Name") << "\"},"
-         << getEnumFeatures(Rec) << ", "
-         << "{\"" << Rec.getValueAsString("DefaultMarch") << "\"})\n";
+         << getEnumFeatures(XLen) << ", "
+         << "{\"" << MArch << "\"})\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 MapTy &Def : Map) {
     const Record &Rec = *(Def.second);
     if (Rec.isSubClassOf("RISCVTuneProcessorModel"))
       OS << "TUNE_PROC(" << Rec.getName() << ", "
          << "\"" << Rec.getValueAsString("Name") << "\")\n";
   }
 
   OS << "\n#undef TUNE_PROC\n";
 }