[llvm-dev] Relation between Register and MCRegister

Tue Sep 29 11:13:18 PDT 2020

On Tue, Sep 29, 2020 at 11:09 AM Daniel Sanders <daniel_l_sanders at apple.com>
wrote:

> Yes so long as you're including the invalid space too (IIRC it matters for
> DBG_VALUE in particular) the reason I didn't do that is that there's a lot
> more ctors than consumers of MCRegister. It seemed cheaper to do the checks
> when they're consumed and pretty much every consumer I encountered started
> with `assert(Reg.isPhysicalRegister() && ...)`.
>

Not sure I follow - asserts are elided in release builds - or is there a
different cost?

>
> On 29 Sep 2020, at 11:02, Mircea Trofin <mtrofin at google.com> wrote:
>
> Thanks! To test my understanding - we could add asserts in MCRegister ctor
> that the value of the unsigned is, indeed, only in the physical register
> namespace, is that correct?
>
> On Tue, Sep 29, 2020 at 10:49 AM Daniel Sanders <
> daniel_l_sanders at apple.com> wrote:
>
>>
>>
>> On 29 Sep 2020, at 09:28, Quentin Colombet <qcolombet at apple.com> wrote:
>>
>> + Daniel who added the MCRegister class.
>>
>> Ah sorry, I replied too fast.
>> I mixed up MCPhysReg with MCRegister.
>>
>> I was not aware we had such class.
>>
>> From a look at it, MCRegister are essentially the same thing as Register.
>> I am guessing that the difference is Register is used in the CodeGen layer,
>> while MCRegister are used in the MC layer.
>>
>> Until recently Register were just plain unsigned types and we introduced
>> it to have stronger type checking. I don’t know what’s the rationale for
>> MCRegister. It looks redundant to be honest. Maybe it is just solving a
>> layering violation issue.
>>
>>
>> The rationale is the same but it's not just layering. MCRegister does not
>> permit vregs or stack slots. However, it still needs to know what they are
>> in order to ensure it never sees them (see below)
>>
>> As for why MCPhysReg and MCRegister both exist. This is mostly down to
>> lots of API's using unsigned while MCPhysReg provides some code size
>> savings. We don't want to blindly truncate to MCPhysReg because
>> accidentally passing vregs or stack slots will collapse them into physregs
>> making them much harder to debug, particularly if we then zero-extend them
>> again to use them in certain API's. However, we also don't want to widen
>> MCPhysReg to unsigned because that will make the backend tables bigger and
>> most backends don't really need the full range allocated to physical
>> registers
>>
>> Let Daniel comment on that.
>>
>> Cheers,
>> -Quentin
>>
>> On Sep 29, 2020, at 9:12 AM, Mircea Trofin <mtrofin at google.com> wrote:
>>
>>
>>
>> On Tue, Sep 29, 2020 at 9:08 AM Quentin Colombet <qcolombet at apple.com>
>> wrote:
>>
>>> Hi,
>>>
>>> Register can represent virtual or physical registers.
>>> MCRegister can only represent physical registers.
>>>
>>
>> That's what I thought, but MCRegister has some stack slot APIs.
>>
>>
>> That's almost correct. MCRegisters can only be invalid ($noreg) or
>> physical registers. The lack of support for vregs is indicated by the lack
>> of any vreg API's while the lack of support for stack slots is indicated by
>> the lack of all (except one) stack slot API's and is mentioned in a comment:
>> "StackSlot values do not exist in the MC layer, see
>> Register::isStackSlot() for the more information on them."
>>
>> The reason there's still an MCRegister::isStackSlot() function despite
>> that is an implementation detail. The checks for invalid and phys reg
>> historically tested for ==0 and >0 respectively. At the same time
>> conversion from Register to unsigned/MCRegister was just a simple cast so
>> if stack slots ever managed to escape from the Register object into
>> unsigned/MCRegister they'd be incorrectly converted from stack slot to
>> physreg (and one that's out of range for MCPhysReg). We needed to be able
>> to detect stack slots being inappropriately converted to MCRegister and the
>> partitioning of the number space is the same as for Register so it made
>> sense to use the same API instead of inventing a new one.
>>
>> Eventually all Register instances are replaced by a MCRegister.
>>>
>>
>> What happens in that case to the stack slot APIs?
>>
>>
>> Registers that are stack slots are never converted to MCRegister. That's
>> not enforced in the constructor but it is enforced in
>> MCRegister::isPhysicalRegister()
>>
>> Cheers,
>>> -Quentin
>>>
>>> > On Sep 28, 2020, at 5:46 PM, Mircea Trofin via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>> >
>>> > Hello,
>>> >
>>> > I'm trying to understand what the relation between these two types is:
>>> do we need them both? Register seems to be delegating to MCRegister without
>>> owning any new additional responsibilities.
>>>
>>
>> Register provides everything related to vregs and stack slots.
>> Since I last looked at these files it seems a bit more of the number
>> space definitions have been moved to MCRegister by
>> 16dae81edc2 [NFCI] Cleanup range checks in Register/MCRegister
>> I'd intentionally left the vreg bit in Register because vregs weren't a
>> thing to MCRegister and that range was already excluded by the existing
>> code but it makes sense to define the whole number space there so long as
>> MCRegister continues to not supply the API's for kinds of register it
>> doesn't deal with (aside from the one it needs internally to prohibit stack
>> slots).
>>
>> So to summarize the difference between MCRegister and Register:
>>
>>    - MCRegister defines the partitioning of the number space both for
>>    itself and for Register
>>    - MCRegister provides support for invalid and phys regs
>>    - Register is a superset of MCRegister and provides additional
>>    support for vreg and stack slots
>>
>>
>> >
>>> > Thanks!
>>> > _______________________________________________
>>> > LLVM Developers mailing list
>>> > llvm-dev at lists.llvm.org
>>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200929/54eaf970/attachment.html>