This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by craig.topper on Mar 9 2021, 4:54 PM.

Details

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.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 9 2021, 4:54 PM
craig.topper requested review of this revision.Mar 9 2021, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 4:54 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
khchen accepted this revision.Mar 9 2021, 9:26 PM

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

This revision is now accepted and ready to land.Mar 9 2021, 9:26 PM
jrtc27 added inline comments.Mar 10 2021, 6:00 PM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
55–56

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

jrtc27 added inline comments.Mar 10 2021, 6:02 PM
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 added inline comments.Mar 10 2021, 6:06 PM
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"

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

craig.topper requested review of this revision.Mar 11 2021, 10:23 AM
jrtc27 added inline comments.Mar 11 2021, 10:26 AM
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 added inline comments.Mar 11 2021, 11:04 AM
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 added inline comments.Mar 11 2021, 11:13 AM
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 added inline comments.Mar 11 2021, 11:34 AM
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 added inline comments.Mar 11 2021, 11:39 AM
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.

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

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

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

luismarques added inline comments.Mar 14 2021, 4:38 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
66–67

Shouldn't this one also be removed?

Error from RISCVMCTargetDesc.cpp too

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
66–67

Yeah I forgot there were two files.

luismarques accepted this revision.Mar 14 2021, 4:47 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Mar 14 2021, 4:47 PM