[RISCV] Rename FPRs and use Register arithmetic

[luismarques]

Summary

Registers, as currently employed in LLVM, have the following properties:

• You can't rely on a Register object having a specific numerical value;
• You can rely on two consecutively enumerated Registers having consecutive numerical values (or having operator overloads that provide equivalent semantics);
• Register are enumerated using a well-defined algorithm that provides some guarantees about enumeration order, given some reasonable assumptions.

This last point may seem surprising but it is a de facto guarantee that many targets rely on, frequently making use of Register arithmetic based on that assumption. In fact, you can see that multiple targets guard against that assumption being violated:

ARMBaseRegisterInfo.cpp:203
static_assert(ARM::D31 == ARM::D16 + 15, "Register list not consecutive!");

X86FloatingPoint.cpp:130
static_assert(X86::FP6 - X86::FP0 == 6, "sequential regnums");

SparcISelLowering.cpp:189
static_assert(SP::I0 + 7 == SP::I7 && SP::O0 + 7 == SP::O7,

AArch64CollectLOH.cpp:259
static_assert(AArch64::X28 - AArch64::X0 + 3 == N_GPR_REGS, "Number of GPRs");
static_assert(AArch64::W30 - AArch64::W0 + 1 == N_GPR_REGS, "Number of GPRs");

The registers are enumerated by RegisterInfoEmitter.cpp, but the order is set by CodeGenRegBank, which sorts all of the Register records using a LessRecordRegister function object. That comparator uses the following sorting logic:

• The names are split into consecutive alphabetical and numerical parts (e.g. Foo42Bar7 -> [Foo, 42, Bar, 7]);
• The alphabetical parts take precedence over the numerical parts in setting the order;
• The number of parts takes precedence over the content of the parts in setting the order.

That last rule explains some less-than-intuitive orderings, such as X86's R8B appearing after ZMM31.

The RISC-V target also makes use of Register arithmetic when it comes to GPR registers. Unfortunately the sorting algorithm doesn't play particularly well with the names of our FPR Registers, resulting in a less-than-ideal ordering: F0_32, F0_64, F1_32, ..., F31_64. Probably because of this, the RISC-V target had so far relied on explicit tables of FPR Registers, rather than also using arithmetic for them, which is inconsistent, verbose and slightly more costly.

There are several ways to solve this problem:

1. Tweak or customize the sorting algorithm. Adding the customizability infrastructure would probably be non-trivial, but it might be a good approach long-term, since the current approach of having a common but hard-coded algorithm is fairly limiting;

2. Use Register arithmetic consistently, even with the less-than-ideal ordering. The ordering is still fairly reasonable, and the sorting algorithm makes it extremely unlikely that any new registers would disrupt it;

3. Change the Register names. For instance, you can rename F0_32 -> F0_F, F0_64 -> F0_D, etc. This ensures that the registers within the same class are enumerated sequentially, and is consistent with the F/D distinction used in other parts of the target.

EDIT UPDATE: The patch now implements option #3.

This patch implements the option #2, but could be easily changed to adopt option #3.

Code review concerns:

• It might be a good idea to have a single place (e.g. a couple of conversion functions) encapsulating the concept of the F/D registers being enumerated in an interleaved fashion. Yet, none of the existing files seem like an ideal place to put them -- RISCVBaseInfo.h and RISCV.h are okayish options but still not a great match. In any case, I would make that transition in two commits anyway, so this patch does not introduce such utility functions.

[jrtc27]

Hm, I don't particularly like this (or at least the F parts of it); the tables are clear, whereas this seems very brittle.. e.g. when you add Q you have to also modify the places doing arithmetic on F and D registers.

It's worth noting that other targets (Mips and Sparc) call their floating point registers F0-F31 and D0-D31 (and there's also Q0-Q31 for Sparc) even though they're always referred to as %fX in the assembly. That would be an alternative resolution to the sorting issue, that also feels less hacky/surprising compared with the numerical-vs-alphabetical ordering subtleties.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

745

We've lost the assertion here; probably want something like assert(Reg >= RISCV::F0_32 && Reg <= RISCV::F31_32 && Reg % 2 == RISCV::F0_32 % 2);

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2116

For completeness you probably want to prepend RegLo >= RISCV::X0 (or > I guess... though at that point you could then argue for >= RISCV::X10 so I'd just say go with >= RISCV::X0 as the simplest thing that's clearly just an "is this a GPR" check).

[luismarques]

In D67423#1665508, @jrtc27 wrote:

It's worth noting that other targets (Mips and Sparc) call their floating point registers F0-F31 and D0-D31 (and there's also Q0-Q31 for Sparc) even though they're always referred to as %fX in the assembly. That would be an alternative resolution to the sorting issue, that also feels less hacky/surprising compared with the numerical-vs-alphabetical ordering subtleties.

Yeah, that's option #3 (with a different naming suggestion). I'm all for that, especially if there's a good precedent -- an existing naming convention for us to be consistent with.

[luismarques]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2116

That wasn't a check for GPRness, but instead that the + 1 didn't overflow the register class (if you were implementing some weird ABI, I guess).

[luismarques]

• Changes the FPRs Register identifiers, to ensure sequential enumeration:
  • F0_32-F31_32 -> F0_F-F31_F
  • F0_64-F31_64 -> F0_D-F31_D
  • These were chosen instead of Fxx/Dxx to better match the ISA spec names, although both options were reasonable.
• Now makes straighforward use of FPR Register arithmetic.
• Adds static asserts regarding the register ordering.
• Updates the FPR register matching, since the tablegen matches now default to the 64-bit regs.
• Tightens some RegNo bounds checks that were too loose on master.
• Updates the title to reflect the changed scope of the patch.

[lenary]

This is looking good. I have just one question this time.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

31 ↗

(On Diff #219958)

Why does this static assert not cover all 31 64-bit FP regs?

[luismarques]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

31 ↗

(On Diff #219958)

You'd need a static_foreach to really check that all 32 of them are consecutive. Checking R31 == R0 + 31 strictly speaking doesn't check that R1-30 aren't scrambled. Choosing different indices for that last assert was a compromise attempt to both impart that concept and to take the opportunity to check some different indices. There's a precedent for checking other regs than the first and last.

[lenary]

One suggestion, then LGTM

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

31 ↗

(On Diff #219958)

It just looks super weird. How about one static assert to ensure that F31_D == F0_D + 31, and then another three asserts to ensure F1_D = F0_D + 1 for each of the register types. I think worrying that the order is scrambled beyond that is way too conservative.

[jrtc27]

I'm still personally not fond of the Fx_F/Fx_D convention. The fact that F0_F < F1_F < F0_D < F1_D is highly surprising, especially when F0_32 < F0_64 < F1_32 < F1_64, and not what one would naively assume. I also don't know whether this peculiarity of TableGen is meant to be something that's relied upon. I know it doesn't match the ISA convention of always calling them Fx, but Fx and Dx have precedence in other backends and do not risk this same confusion. Moreover, the spec itself is highly inconsistent in how to refer to single and double precision floating point:

• The load/store instructions are suffixed with W/D/Q
• Other floating point instructions are suffixed with .S/.D/.Q
• The extensions are F/D/Q

So, personally, I would still vote for Fx/Dx/Qx given it avoids confusion, matches a bunch of other backends, and matches the ISA extension letters.

[asb]

In D67423#1671595, @jrtc27 wrote:

I'm still personally not fond of the Fx_F/Fx_D convention. The fact that F0_F < F1_F < F0_D < F1_D is highly surprising, especially when F0_32 < F0_64 < F1_32 < F1_64, and not what one would naively assume. I also don't know whether this peculiarity of TableGen is meant to be something that's relied upon. I know it doesn't match the ISA convention of always calling them Fx, but Fx and Dx have precedence in other backends and do not risk this same confusion. Moreover, the spec itself is highly inconsistent in how to refer to single and double precision floating point:

I think with this patch, it's actually the case that F0_D is less than F0_F. I don't think it makes sense to worry about the relative ordering of register classes, just the ordering within a class. It's a shame that RISCVAsmParser has to care, but I think the static_assert is a reasonable solution there. If TableGen were to start doing something less predictable in terms of relative ordering of the register classes, we could always make a more generic function that can handle more conversions than is strictly necessary.

• The load/store instructions are suffixed with W/D/Q
• Other floating point instructions are suffixed with .S/.D/.Q
• The extensions are F/D/Q

So, personally, I would still vote for Fx/Dx/Qx given it avoids confusion, matches a bunch of other backends, and matches the ISA extension letters.

That's a good point, but it seems a shame to lose out on the one thing that the spec is consistent about - the architectural register name. I suppose F0 and F0_D is another alternative.

[luismarques]

• Rebased on master;
• Addresses @lenary's static_assert concerns.

[asb]

This LGTM, thanks. Added a few nits. I think Sam's suggestion of adding a helper function or two to handle the register arithmetic is worth evaluating, but doesn't need to be done in this patch.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

32 ↗

(On Diff #221219)

clang-format wants to wrap this

34 ↗

(On Diff #221219)

clang-format wants to wrap this

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

public:
   }
 };
 } // end anonymous namespace.
 
 #define GET_REGISTER_MATCHER
 #define GET_MATCHER_IMPLEMENTATION
 #include "RISCVGenAsmMatcher.inc"
 
-// Return the matching FPR64 register for the given FPR32.
-// FIXME: Ideally this function could be removed in favour of using
-// information from TableGen.
 static Register convertFPR32ToFPR64(Register Reg) {
-  switch (Reg) {
-  default:
-    llvm_unreachable("Not a recognised FPR32 register");
-  case RISCV::F0_32: return RISCV::F0_64;
-  case RISCV::F1_32: return RISCV::F1_64;
-  case RISCV::F2_32: return RISCV::F2_64;
-  case RISCV::F3_32: return RISCV::F3_64;
-  case RISCV::F4_32: return RISCV::F4_64;
-  case RISCV::F5_32: return RISCV::F5_64;
-  case RISCV::F6_32: return RISCV::F6_64;
-  case RISCV::F7_32: return RISCV::F7_64;
-  case RISCV::F8_32: return RISCV::F8_64;
-  case RISCV::F9_32: return RISCV::F9_64;
-  case RISCV::F10_32: return RISCV::F10_64;
-  case RISCV::F11_32: return RISCV::F11_64;
-  case RISCV::F12_32: return RISCV::F12_64;
-  case RISCV::F13_32: return RISCV::F13_64;
-  case RISCV::F14_32: return RISCV::F14_64;
-  case RISCV::F15_32: return RISCV::F15_64;
-  case RISCV::F16_32: return RISCV::F16_64;
-  case RISCV::F17_32: return RISCV::F17_64;
-  case RISCV::F18_32: return RISCV::F18_64;
-  case RISCV::F19_32: return RISCV::F19_64;
-  case RISCV::F20_32: return RISCV::F20_64;
-  case RISCV::F21_32: return RISCV::F21_64;
-  case RISCV::F22_32: return RISCV::F22_64;
-  case RISCV::F23_32: return RISCV::F23_64;
-  case RISCV::F24_32: return RISCV::F24_64;
-  case RISCV::F25_32: return RISCV::F25_64;
-  case RISCV::F26_32: return RISCV::F26_64;
-  case RISCV::F27_32: return RISCV::F27_64;
-  case RISCV::F28_32: return RISCV::F28_64;
-  case RISCV::F29_32: return RISCV::F29_64;
-  case RISCV::F30_32: return RISCV::F30_64;
-  case RISCV::F31_32: return RISCV::F31_64;
-  }
+  return Reg + 1; // F0_32, F0_64, F1_32, F1_64, ...
 }
 
 unsigned RISCVAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
                                                     unsigned Kind) {
   RISCVOperand &Op = static_cast<RISCVOperand &>(AsmOp);
   if (!Op.isReg())
     return Match_InvalidOperand;
 

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

 extern "C" void LLVMInitializeRISCVDisassembler() {
   // Register the disassembler for each target.
   TargetRegistry::RegisterMCDisassembler(getTheRISCV32Target(),
                                          createRISCVDisassembler);
   TargetRegistry::RegisterMCDisassembler(getTheRISCV64Target(),
                                          createRISCVDisassembler);
 }
 
-static const Register GPRDecoderTable[] = {
-  RISCV::X0,  RISCV::X1,  RISCV::X2,  RISCV::X3,
-  RISCV::X4,  RISCV::X5,  RISCV::X6,  RISCV::X7,
-  RISCV::X8,  RISCV::X9,  RISCV::X10, RISCV::X11,
-  RISCV::X12, RISCV::X13, RISCV::X14, RISCV::X15,
-  RISCV::X16, RISCV::X17, RISCV::X18, RISCV::X19,
-  RISCV::X20, RISCV::X21, RISCV::X22, RISCV::X23,
-  RISCV::X24, RISCV::X25, RISCV::X26, RISCV::X27,
-  RISCV::X28, RISCV::X29, RISCV::X30, RISCV::X31
-};
-
 static DecodeStatus DecodeGPRRegisterClass(MCInst &Inst, uint64_t RegNo,
                                            uint64_t Address,
                                            const void *Decoder) {
   const FeatureBitset &FeatureBits =
       static_cast<const MCDisassembler *>(Decoder)
           ->getSubtargetInfo()
           .getFeatureBits();
   bool IsRV32E = FeatureBits[RISCV::FeatureRV32E];
 
-  if (RegNo > array_lengthof(GPRDecoderTable) || (IsRV32E && RegNo > 15))
+  if (RegNo > 32 || (IsRV32E && RegNo > 15))
     return MCDisassembler::Fail;
 
-  // We must define our own mapping from RegNo to register identifier.
-  // Accessing index RegNo in the register class will work in the case that
-  // registers were added in ascending order, but not in general.
-  Register Reg = GPRDecoderTable[RegNo];
+  Register Reg = RISCV::X0 + RegNo;
   Inst.addOperand(MCOperand::createReg(Reg));
   return MCDisassembler::Success;
 }
 
-static const Register FPR32DecoderTable[] = {
-  RISCV::F0_32,  RISCV::F1_32,  RISCV::F2_32,  RISCV::F3_32,
-  RISCV::F4_32,  RISCV::F5_32,  RISCV::F6_32,  RISCV::F7_32,
-  RISCV::F8_32,  RISCV::F9_32,  RISCV::F10_32, RISCV::F11_32,
-  RISCV::F12_32, RISCV::F13_32, RISCV::F14_32, RISCV::F15_32,
-  RISCV::F16_32, RISCV::F17_32, RISCV::F18_32, RISCV::F19_32,
-  RISCV::F20_32, RISCV::F21_32, RISCV::F22_32, RISCV::F23_32,
-  RISCV::F24_32, RISCV::F25_32, RISCV::F26_32, RISCV::F27_32,
-  RISCV::F28_32, RISCV::F29_32, RISCV::F30_32, RISCV::F31_32
-};
-
 static DecodeStatus DecodeFPR32RegisterClass(MCInst &Inst, uint64_t RegNo,
                                              uint64_t Address,
                                              const void *Decoder) {
-  if (RegNo > array_lengthof(FPR32DecoderTable))
+  if (RegNo > 32)
     return MCDisassembler::Fail;
 
-  // We must define our own mapping from RegNo to register identifier.
-  // Accessing index RegNo in the register class will work in the case that
-  // registers were added in ascending order, but not in general.
-  Register Reg = FPR32DecoderTable[RegNo];
+  Register Reg = RISCV::F0_32 + RegNo * 2; // F0_32, F0_64, F1_32, F1_64, ...
   Inst.addOperand(MCOperand::createReg(Reg));
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeFPR32CRegisterClass(MCInst &Inst, uint64_t RegNo,
                                               uint64_t Address,
                                               const void *Decoder) {
   if (RegNo > 8) {
     return MCDisassembler::Fail;
   }
-  Register Reg = FPR32DecoderTable[RegNo + 8];
+  Register Reg = RISCV::F8_32 + RegNo * 2; // F8_32, F8_64, F9_32, F9_64, ...
   Inst.addOperand(MCOperand::createReg(Reg));
   return MCDisassembler::Success;
 }
 
-static const Register FPR64DecoderTable[] = {
-  RISCV::F0_64,  RISCV::F1_64,  RISCV::F2_64,  RISCV::F3_64,
-  RISCV::F4_64,  RISCV::F5_64,  RISCV::F6_64,  RISCV::F7_64,
-  RISCV::F8_64,  RISCV::F9_64,  RISCV::F10_64, RISCV::F11_64,
-  RISCV::F12_64, RISCV::F13_64, RISCV::F14_64, RISCV::F15_64,
-  RISCV::F16_64, RISCV::F17_64, RISCV::F18_64, RISCV::F19_64,
-  RISCV::F20_64, RISCV::F21_64, RISCV::F22_64, RISCV::F23_64,
-  RISCV::F24_64, RISCV::F25_64, RISCV::F26_64, RISCV::F27_64,
-  RISCV::F28_64, RISCV::F29_64, RISCV::F30_64, RISCV::F31_64
-};
-
 static DecodeStatus DecodeFPR64RegisterClass(MCInst &Inst, uint64_t RegNo,
                                              uint64_t Address,
                                              const void *Decoder) {
-  if (RegNo > array_lengthof(FPR64DecoderTable))
+  if (RegNo > 32)
     return MCDisassembler::Fail;
 
-  // We must define our own mapping from RegNo to register identifier.
-  // Accessing index RegNo in the register class will work in the case that
-  // registers were added in ascending order, but not in general.
-  Register Reg = FPR64DecoderTable[RegNo];
+  Register Reg = RISCV::F0_64 + RegNo * 2; // F0_32, F0_64, F1_32, F1_64, ...
   Inst.addOperand(MCOperand::createReg(Reg));
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeFPR64CRegisterClass(MCInst &Inst, uint64_t RegNo,
                                               uint64_t Address,
                                               const void *Decoder) {
   if (RegNo > 8) {
     return MCDisassembler::Fail;
   }
-  Register Reg = FPR64DecoderTable[RegNo + 8];
+  Register Reg = RISCV::F8_64 + RegNo * 2; // F8_32, F8_64, F9_32, F9_64, ...
   Inst.addOperand(MCOperand::createReg(Reg));
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeGPRNoX0RegisterClass(MCInst &Inst, uint64_t RegNo,
                                                uint64_t Address,
                                                const void *Decoder) {
   if (RegNo == 0) {

 }
 
 static DecodeStatus DecodeGPRCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,
                                             const void *Decoder) {
   if (RegNo > 8)
     return MCDisassembler::Fail;
 
-  Register Reg = GPRDecoderTable[RegNo + 8];
+  Register Reg = RISCV::X8 + RegNo;
   Inst.addOperand(MCOperand::createReg(Reg));
   return MCDisassembler::Success;
 }
 
 // Add implied SP operand for instructions *SP compressed instructions. The SP
 // operand isn't explicitly encoded in the instruction.
 static void addImplySP(MCInst &Inst, int64_t Address, const void *Decoder) {
   if (Inst.getOpcode() == RISCV::C_LWSP || Inst.getOpcode() == RISCV::C_SWSP ||

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

if (IsF64OnRV32DSoftABI && VA.isRegLoc()) {
         // Work out the address of the stack slot.
         if (!StackPtr.getNode())
           StackPtr = DAG.getCopyFromReg(Chain, DL, RISCV::X2, PtrVT);
         // Emit the store.
         MemOpChains.push_back(
             DAG.getStore(Chain, DL, Hi, StackPtr, MachinePointerInfo()));
       } else {
         // Second half of f64 is passed in another GPR.
+        assert(RegLo < RISCV::X31 && "Invalid register pair");
         Register RegHigh = RegLo + 1;
         RegsToPass.push_back(std::make_pair(RegHigh, Hi));
       }
       continue;
     }
 
     // IsF64OnRV32DSoftABI && VA.isMemLoc() is handled below in the same way
     // as any other MemLoc.