[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:986
+    return error(Twine("unknown register name '") + Name + "'");
+
+  lex();
----------------
plotfi wrote:
> thegameg wrote:
> > plotfi wrote:
> > > thegameg wrote:
> > > > I think the code would be much simpler if we do the same thing as parseVirtualRegister here. By that I mean call createIncompleteVirtualRegister, create a VRegInfo object without any reg class, then let the rest of the code parse the reg class and follow the usual path. I also think that you don't need to explicitly parse the reg class in this function since the user will get an error like `Cannot determine class/bank of virtual register` if no reg class is specified at def time anyway. This would allow you to get rid of the other change you have where you conditionally lex things.
> > > > 
> > > > Please let me know if what I'm saying doesn't make sense in this case as I haven't really tried to implement this.
> > > I don't understand this completely, but I think one thing I could do is add a name parameter to createIncompleteVirtualRegister and to getVRegInfo and you're saying parseRegisterClassOrBank will just finish building the vreg for me?
> > What I mean by that is:
> > * the only way to parse a named virtual register is through `parseRegisterOperand`
> > * `parseRegisterOperand` handles all kinds of registers, sub registers, register classes / banks.
> > * (1) the only thing `parseVirtualRegister` does is create an empty `VRegInfo` and create an "incomplete" virtual register.
> > * (2) the call to `parseRegisterClassOrBank` in `parseRegisterOperand` will continue the parsing.
> > 
> > This is the part where I am not completely sure:
> > * can you do the same as (1), with additional steps as inserting the name into `Names2VRegs` and `MRI::VReg2Name`
> > * will re-using (2) work with no additional changes here?
> Actually the only reason I created parseVirtualRegister was to use Names2VRegs. Since we use a different sigil for the physregs I think we should use a different table to map the names. 
> 
> 
Yes that makes sense. We should be able to use `%eax` and `$eax` that map to different registers.


Repository:
  rL LLVM

https://reviews.llvm.org/D43916