[PATCH] D43916: Named VReg support for MIR

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:91
 VRegInfo &PerFunctionMIParsingState::getVRegInfo(unsigned Num) {
-  auto I = VRegInfos.insert(std::make_pair(Num, nullptr));
+  MachineRegisterInfo &MRI = MF.getRegInfo();
+  const bool isNamed = TargetRegisterInfo::isVirtualRegister(Num) &&
----------------
I still think this is a bit confusing because sometimes `Num` is a register, sometimes it's an ID. Why not make a separate function where you do what you do in `getVirtualRegisterByName` + what's needed from here (something like `VRegInfo &PerFunctionMIParsingState::getVRegInfo(StringRef Num)`).


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:2534
+    if (!Names2VRegs.insert(std::make_pair(RegName, Reg)).second)
+      return error(Twine("Invalid register name '") + RegName + "'");
+    return Reg;
----------------
It's a little unclear how to differentiate from an error (result = 1) or a register with the value 1. How about inlining this in the caller since it's only used once?


================
Comment at: lib/CodeGen/MIRParser/MIRParser.cpp:519
+  };
+  for (auto VRegInfos : VRegInfosMaps) {
+    for (auto P : *VRegInfos) {
----------------
```
for (auto *VRegInfos : {&PFS.VRegInfos, &PFS.VRegInfosNamed})
```
should do the trick.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:176
     TheDelegate->MRI_NoteNewVirtualRegister(Reg);
+  insertVRegByName(Name, Reg);
   return Reg;
----------------
Not needed anymore, right?


Repository:
  rL LLVM

https://reviews.llvm.org/D43916