[PATCH] D43916: Named VReg support for MIR

[via llvm-commits]

> 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.

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