[RISCV] Remap 'generic' CPU to 'generic-rv32' or 'generic-rv64'. Validate 64Bit feature against the triple.

[craig.topper]

Summary

I encountered a project that uses llvm that passes "generic" by
default. While I could fix that project, I wouldn't be surprised
if other projects did something similar. So it seems like
a good idea to be defensive.

I've also added validation of the 64Bit feature against the
triple so that we can catch a mismatched CPU before failing in
a mysterious way. We can make it pretty far in isel because we
calculate XLenVT from the triple and use that to set up the legal
integer type.

[khchen]

We could see almost all other targets support "generic" CPU, so I think it's LGTM.

[jrtc27]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

Should we be mapping -mtune=generic to generic-rv$XLEN too?

[jrtc27]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

(For consistency, and although it seems like a stupid thing to do, maybe you want to override a non-generic default back to generic)

[craig.topper]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

i'm not sure what you mean with "maybe you want to override a non-generic default back to generic"

[craig.topper]

Sanitize the tune CPU as well. Get rid of some std::strings that don't need to exist.

[jrtc27]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

Oh as in either your system toolchain decides to bake in a different default or you're building something that already does -mtune=rocket-rv$XLEN and you want to override it back to -mtune=generic by adding that to CFLAGS.

61

Huh, TIL this has been added. Is there a reason not to use this for CPU? I guess because we want a version that returns llvm::Optional?

[craig.topper]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

This already works because clang calls RISCV::resolveTuneCPUAlias. Clang should only ever be sending valid names for both the CPU and tune CPU. If it doesn't we have a poor user experience where messages about things being ignore will print from the subtarget parsing code.

[jrtc27]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

Then I don't think I understand why this code needs to handle plain "generic" at all if Clang should have rewritten it to the right XLEN-specific one?

[craig.topper]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

Because I was working with a project that is not clang, that passed "generic" by default to the TargetMachine constructor. The subtarget parsing code printed a warning, but everything blissfully went on until it crashed in the middle of isel in a non-obvious way. I was trying to improve the experience here because nearly every target supports "generic" in llc so I thought I would make RISCV more cooperative.

Even with everything I've done here a warning still gets printed about the CPU being ignored even before it gets to this code due the MCSubtargetInfo constructor also parsing feature only to have the RISCVSubtarget constructor reparse them. I hadn't realized that earlier. So I'm not sure it makes sense to sanitize arbitrary CPU names since it will still print a warning.

So I'd like to start over and do one of these things

1. Just add the error checking in RISCVFeatures::validate to give a better error than maybe failing in isel.
2. My original patch just remap "generic" since it could be a common mistake to make. And add the error checking to RISCVFeatures::validate

[jrtc27]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

55–56

Ah ok that clears it up. I agree that supporting "generic" ~everywhere is a good idea, and tbh similarly any foo where we have foo-rv32 and foo-rv64 ideally (e.g. rocket), though those are going to be used much more rarely so matter less and depending on how you do it can be more annoying to support than a single special case so I don't think it's _necessary_ in the same way.

[craig.topper]

Go back to my original patch plus the std::string cleanup. Add explanation about why I've done it this way.

[luismarques]

Why not just error out if passed "generic"? The error message could indicate that "generic-rv<XLEN>" should be used instead.
In other words, it's not clear to me what we're getting with the remap, particularly since it still prints a warning...?

[craig.topper]

Given an error with a suggestion to use generic-rv32 or generic-rv64 instead.

[luismarques]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

66–67

Shouldn't this one also be removed?

[craig.topper]

Error from RISCVMCTargetDesc.cpp too

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

66–67

Yeah I forgot there were two files.

[luismarques]

LGTM. Thanks!

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp

 // Returns the register holding shadow call stack pointer.
 MCRegister getSCSPReg() { return RISCV::X18; }
 
 } // namespace RISCVABI
 
 namespace RISCVFeatures {
 
 void validate(const Triple &TT, const FeatureBitset &FeatureBits) {
+  if (TT.isArch64Bit() && !FeatureBits[RISCV::Feature64Bit])
+    report_fatal_error("RV64 target requires an RV64 CPU");
+  if (!TT.isArch64Bit() && FeatureBits[RISCV::Feature64Bit])
+    report_fatal_error("RV32 target requires an RV32 CPU");
   if (TT.isArch64Bit() && FeatureBits[RISCV::FeatureRV32E])
     report_fatal_error("RV32E can't be enabled for an RV64 target");
 }
 
 } // namespace RISCVFeatures
 
 void RISCVVType::printVType(unsigned VType, raw_ostream &OS) {
   RISCVVSEW VSEW = getVSEW(VType);

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

static MCAsmInfo *createRISCVMCAsmInfo(const MCRegisterInfo &MRI,
   MCRegister SP = MRI.getDwarfRegNum(RISCV::X2, true);
   MCCFIInstruction Inst = MCCFIInstruction::cfiDefCfa(nullptr, SP, 0);
   MAI->addInitialFrameState(Inst);
 
   return MAI;
 }
 
 static MCSubtargetInfo *createRISCVMCSubtargetInfo(const Triple &TT,
                                                    StringRef CPU, StringRef FS) {
-  std::string CPUName = std::string(CPU);
-  if (CPUName.empty())
-    CPUName = TT.isArch64Bit() ? "generic-rv64" : "generic-rv32";
-  return createRISCVMCSubtargetInfoImpl(TT, CPUName, /*TuneCPU*/ CPUName, FS);
+  if (CPU.empty())
+    CPU = TT.isArch64Bit() ? "generic-rv64" : "generic-rv32";
+  if (CPU == "generic")
+    report_fatal_error(Twine("CPU 'generic' is not supported. Use ") +
+                       (TT.isArch64Bit() ? "generic-rv64" : "generic-rv32"));
+  return createRISCVMCSubtargetInfoImpl(TT, CPU, /*TuneCPU*/ CPU, FS);
 }
 
 static MCInstPrinter *createRISCVMCInstPrinter(const Triple &T,
                                                unsigned SyntaxVariant,
                                                const MCAsmInfo &MAI,
                                                const MCInstrInfo &MII,
                                                const MCRegisterInfo &MRI) {
   return new RISCVInstPrinter(MAI, MII, MRI);

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

 static cl::opt<unsigned> RVVVectorLMULMax(
     "riscv-v-fixed-length-vector-lmul-max",
     cl::desc("The maximum LMUL value to use for fixed length vectors. "
              "Fractional LMUL values are not supported."),
     cl::init(8), cl::Hidden);
 
 void RISCVSubtarget::anchor() {}
 
-RISCVSubtarget &RISCVSubtarget::initializeSubtargetDependencies(
-    const Triple &TT, StringRef CPU, StringRef TuneCPU, StringRef FS, StringRef ABIName) {
+RISCVSubtarget &
+RISCVSubtarget::initializeSubtargetDependencies(const Triple &TT, StringRef CPU,
+                                                StringRef TuneCPU, StringRef FS,
+                                                StringRef ABIName) {
   // Determine default and user-specified characteristics
   bool Is64Bit = TT.isArch64Bit();
-  std::string CPUName = std::string(CPU);
-  std::string TuneCPUName = std::string(TuneCPU);
-  if (CPUName.empty())
-    CPUName = Is64Bit ? "generic-rv64" : "generic-rv32";
-  if (TuneCPUName.empty())
-    TuneCPUName = CPUName;
-  ParseSubtargetFeatures(CPUName, TuneCPUName, FS);
+  if (CPU.empty())
+    CPU = Is64Bit ? "generic-rv64" : "generic-rv32";
+  if (CPU == "generic")
+    report_fatal_error(Twine("CPU 'generic' is not supported. Use ") +
+                       (Is64Bit ? "generic-rv64" : "generic-rv32"));
+
+  if (TuneCPU.empty())
+    TuneCPU = CPU;
+
+  ParseSubtargetFeatures(CPU, TuneCPU, FS);
   if (Is64Bit) {
     XLenVT = MVT::i64;
     XLen = 64;
   }
 
   TargetABI = RISCVABI::computeTargetABI(TT, getFeatureBits(), ABIName);
   RISCVFeatures::validate(TT, getFeatureBits());
   return *this;