[PATCH] D43916: Named VReg support for MIR

[Francis Visoiu Mistrih via llvm-commits]

I think it’s just a matter of using SourceMgr::DK_Warning instead of SourceMgr::DK_Error when constructing an SMDiagnostic in MIParser::error. I don’t think printing a message on errs() is a good idea.

Thanks,

— 
Francis

> On Mar 15, 2018, at 9:23 PM, Puyan Lotfi <puyan at puyan.org> wrote:
> 
> Strange, MIParser.cpp doesn't appear to have a mechanism for warnings. I only see errors. Could I just print a message on errs() and work on a warning patch separately?
> 
> PL
> 
> 
> ​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.
>> 
>>> Repository:
>>> 
>>> rL LLVM
>>> 
>>> https://reviews.llvm.org/D43916
> 
>