[PATCH] D43916: Named VReg support for MIR

[Puyan Lotfi via llvm-commits]

​Sent with ProtonMail Secure Email.​

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On March 15, 2018 9:25 AM, <qcolombet at apple.com> wrote:

> > On Mar 15, 2018, at 8:00 AM, Francis Visoiu Mistrih via Phabricator reviews at reviews.llvm.org wrote:
> > 
> > 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.
> 
> Could we have a warning (that we can disable) when someone does something like that?
> 
> I believe it is something easily missable.
> 

I think that could be done. Just use the Names2Regs map to look up names. If they match then print a warning. I'm pretty sure Names2Regs gets preallocated in advanced. 

PL

> > Repository:
> > 
> > rL LLVM
> > 
> > https://reviews.llvm.org/D43916