[PATCH] D67423: [RISCV] Consistently use Register arithmetic

[Luís Marques via Phabricator via llvm-commits]

luismarques created this revision.
luismarques added reviewers: asb, lenary.
Herald added subscribers: llvm-commits, pzheng, s.egerton, Jim, benna, psnobl, jocewei, PkmX, rkruppe, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, MaskRay, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, apazos, simoncook, johnrusso, rbar, fedor.sergeev, hiraditya, kristof.beyls, jyknight.
Herald added a project: LLVM.

`Register`s, 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 `Register`s 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 `Register`s, 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 `Register`s, 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.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67423

Files:
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
  llvm/lib/Target/RISCV/RISCVISelLowering.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D67423.219614.patch
Type: text/x-patch
Size: 7811 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190911/f15b328a/attachment-0001.bin>