[PATCH] D43916: Named VReg support for MIR

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added a comment.

Thanks for the patch!

Few remarks:

- The named vregs are not mapped back to the same vreg id. Take this example:

  ---
  name:            Proc
  registers:
    - { id: 0, class: gpr32, preferred-register: '' }
  body:             |
    bb.0:
      %foo:gpr64 = IMPLICIT_DEF

This will give us this:

  registers:
    - { id: 0, class: gpr32, preferred-register: '' }
    - { id: 1, class: gpr64, preferred-register: '' }
    - { id: 2, class: gpr64, preferred-register: '' }

- There is no way to specify something like a preferred-register for the named regs, or explicitly force a class on it.

- Some of the remarks in comments.

I think it's good approach to keep an unique ID, since it doesn't request much change from MachineRegisterInfo.



================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:434
 
+  std::string getVRegName(unsigned Reg) const {
+    return VReg2Name.inBounds(Reg) ? VReg2Name[Reg] : "";
----------------
StringRef might be better here?


================
Comment at: include/llvm/CodeGen/TargetRegisterInfo.h:1155
+                   unsigned SubRegIdx = 0,
+                   const MachineRegisterInfo *MRI = nullptr);
 
----------------
Should we use `MRI->getTargetRegisterInfo` and only pass MRI to printReg?


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:968
+
+  const TargetRegisterClass *RC = nullptr;
+  for (unsigned i = 0; i < TRI->getNumRegClasses(); i++) {
----------------
Should probably first look in PFS.Names2RegBanks or just re-use `parseRegisterClassOrRegBank`.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:982
+
+  Reg = MRI.createVirtualRegister(RC, Name.str());
+  bool WasInserted =
----------------
You're first creating a virtual register, then you're calling `PFS.getVRegInfo(Reg)` at the end of the function, which creates the VRegInfo by also creating another virtual register.

Basically for this:

```
---
name:            Proc
tracksRegLiveness: true
body:             |
  bb.0.bb:
    %foo:gpr64 = IMPLICIT_DEF
...
```

you end up with
```
registers:
  - { id: 0, class: gpr64, preferred-register: '' }
  - { id: 1, class: gpr64, preferred-register: '' }
```


================
Comment at: lib/CodeGen/TargetRegisterInfo.cpp:97
+    else if (TargetRegisterInfo::isVirtualRegister(Reg)) {
+      std::string name = MRI->getVRegName(Reg);
+      if (name != "")
----------------
StringRef here too?


Repository:
  rL LLVM

https://reviews.llvm.org/D43916