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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:58-59
     CPUName = Is64Bit ? "generic-rv64" : "generic-rv32";
   if (TuneCPUName.empty())
     TuneCPUName = CPUName;
   ParseSubtargetFeatures(CPUName, TuneCPUName, FS);
----------------
jrtc27 wrote:
> craig.topper wrote:
> > jrtc27 wrote:
> > > craig.topper wrote:
> > > > jrtc27 wrote:
> > > > > jrtc27 wrote:
> > > > > > Should we be mapping -mtune=generic to generic-rv$XLEN too?
> > > > > (For consistency, and although it seems like a stupid thing to do, maybe you want to override a non-generic default back to generic)
> > > > i'm not sure what you mean with "maybe you want to override a non-generic default back to generic"
> > > 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.
> > 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.
> 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?
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 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98307/new/

https://reviews.llvm.org/D98307