[PATCH] D43916: Named VReg support for MIR

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:434
 
+  std::string getVRegName(unsigned Reg) const {
+    return VReg2Name.inBounds(Reg) ? VReg2Name[Reg] : "";
----------------
plotfi wrote:
> plotfi wrote:
> > thegameg wrote:
> > > StringRef might be better here?
> > Yeah. I'll update the patch.
> Tried stringref, had some bugs in debug mode. Changed it back. 
What was the issue?


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:711
+  unsigned createVirtualRegister(const TargetRegisterClass *RegClass,
+                                 std::string name = "");
 
----------------
`Name` instead of `name`.


================
Comment at: include/llvm/CodeGen/TargetRegisterInfo.h:1155
+                   unsigned SubRegIdx = 0,
+                   const MachineRegisterInfo *MRI = nullptr);
 
----------------
plotfi wrote:
> thegameg wrote:
> > Should we use `MRI->getTargetRegisterInfo` and only pass MRI to printReg?
> Oh! that sounds pretty good. Hmm. The question is, does all the calling code where we call printReg have an MRI. From my initial survey that doesn't appear to be the case. 
You should be able to grab an MRI at any point but I guess it's not worth changing it everywhere. Maybe in a future patch.


================
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;
----------------
plotfi wrote:
> bogner wrote:
> > 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.
> In the case of the vreg assignment it always parses it, in the case of a use it doesn't need to. For example:
> 
> %foo:gpr32 = ...
> $5 = ADD %foo, %foo:gpr32
> 
> Hmm, I should probably make it never parse the regclass in the case of use. 
I don't think you should special-case this here. We can also have regclasses repeated on uses. What about using something like:

```
VRegInfo &PerFunctionMIParsingState::getVRegInfo(StringRef Name)
```

and follow the way non-named vregs are created.


Repository:
  rL LLVM

https://reviews.llvm.org/D43916