[PATCH] D43916: Named VReg support for MIR

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:79
 
+  /// VReg2Name - Map for recovering vreg name from vreg number.
+  ///
----------------
Don't repeat the name in the comment.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:86-88
+  /// VRegNames - StringSet that is used to unique the vreg names.
+  ///
+  /// Used to prevent vreg names from being reused.
----------------
No need to repeat yourself, one sentence will do here.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:1202-1207
+  bool isNamedVreg = Token.is(MIToken::NamedVirtualRegister);
   if (parseRegister(Reg, RegInfo))
     return true;
-  lex();
+  if (isNamedVreg && !Token.is(MIToken::NamedVirtualRegister)) {
+    if (parseRegisterClassOrBank(*RegInfo))
+        return true;
----------------
This doesn't seem right. Parsing a named virtual register should either always or never parse the register class. As is, it seems that it sometimes does, so you have to do this complicated dance here.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:165
+MachineRegisterInfo::createVirtualRegister(const TargetRegisterClass *RegClass,
+                                           std::string name) {
   assert(RegClass && "Cannot create register without RegClass!");
----------------
Here too, name can probably be a StringRef (also it should be called Name) - we'll happily create a std::string if/when we actually store it in VRegNames.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:180
+    VRegNames.insert(name);
+    // VReg2Name.insert(std::pair<unsigned, std::string>(Reg, name));
+    VReg2Name.grow(Reg);
----------------
Don't leave commented out code laying around, please.


Repository:
  rL LLVM

https://reviews.llvm.org/D43916