Relation between Register and MCRegister

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.

Thanks!

Hi,

Register can represent virtual or physical registers.
MCRegister can only represent physical registers.

Eventually all Register instances are replaced by a MCRegister.

Cheers,
-Quentin

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.

Eventually all Register instances are replaced by a MCRegister.

What happens in that case to the stack slot APIs?

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

Let Daniel comment on that.

Cheers,
-Quentin

  • 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

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

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! 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?

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() && ...).

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?

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?

Even though the release builds elide them there’s still a cost to the compiler engineers using asserts builds for their daily development.

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?

Even though the release builds elide them there’s still a cost to the compiler engineers using asserts builds for their daily development.

Just to make sure I’m seeing things the same way - converting a MCRegister to Register is correct; converting a Register to a MCRegister is not always correct. It’s only correct if the Register’s underlying unsigned value maps to the physical namespace (or 0).

Same for a 32 bit unsigned value.

Does this fit in the design intent behind Register / MCRegister?

Thanks!

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?

Even though the release builds elide them there’s still a cost to the compiler engineers using asserts builds for their daily development.

Just to make sure I’m seeing things the same way - converting a MCRegister to Register is correct; converting a Register to a MCRegister is not always correct. It’s only correct if the Register’s underlying unsigned value maps to the physical namespace (or 0).

That’s right.

Same for a 32 bit unsigned value.

When converting 32-bit unsigned to MCRegister/Register, yes it’s the same. When converting MCRegister/Register to unsigned we can’t know if it’s correct or not as it depends on how it’s used later. In general, converting to unsigned is an indication that the consumer needs to adopt MCRegister/Register as appropriate for their usage.

Does this fit in the design intent behind Register / MCRegister?

Yep

Cool, thanks! I’ll start a few CLs then that start enforcing this, with the final goal of removing those implicit conversions.

We could, after that, consider adding asserts in the ctor of MCRegister, or in those conversions - noting the tradeoff you mentioned earlier (arguably a good tradeoff, to catch errors early, etc). But we can cross that bridge when we get there.