[PATCH] D65552: Move llvm::Register from CodeGen to MC to correct a layering issue

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In D65552#1609532 <https://reviews.llvm.org/D65552#1609532>, @arsenm wrote:

> In D65552#1609531 <https://reviews.llvm.org/D65552#1609531>, @dsanders wrote:
>
> > In D65552#1609530 <https://reviews.llvm.org/D65552#1609530>, @arsenm wrote:
> >
> > > In D65552#1609529 <https://reviews.llvm.org/D65552#1609529>, @dsanders wrote:
> > >
> > > > MCRegisterClass::contains() is the main example I have at the moment. It's often called with virtual register arguments and returns false for this case.
> > >
> > >
> > > This sounds really broken. I would expect that to be an illegal call
> >
> >
> > Are you referring to calling TargetRegisterClass::contains() on vregs to begin with or passing them on to MCRegisterClass::contains?
> >  The former is happening everywhere as far as I can see but we could practically make the latter true.
>
>
> Both. It doesn't make any sense to pass a virtual register to either. For a virtual register you would check the register class from MachineRegisterInfo


I can see some sense to the former as a virtual register is never a member of a register class. It will usually give the intended result for passes that act on physical registers.

At this point, here's my plan for these couple patches:

- Make https://reviews.llvm.org/D65553 independent of this patch so that we have the correct namespacing in `Register`
- Move the definition of the physical register namespace to a stub MCRegister class to allow us to start checking for vregs/stack-slots in the MC layer. Making an actual MCRegister to replace is a separate piece of work (I have another patch that changes various `unsigned` to `MCPhysReg` in MCRegisterInfo and it would make more sense to go along with that)
- Add a `if (!isPhysical()) return false` to TargetRegisterInfo::contains() to prevent non-physreg's leaking into the MC layer by that method

I don't want to take on sorting out the callers of TargetRegisterInfo::contains() though I think that's going to be a rabbit hole and I'm trying to get back to that SrcOp patch :-) (the reason I looked into this on the way is the implicit cast operator from Register to unsigned can still give some easy routes to accidentally mix up immediates and registers, I was looking into the difficulty of requiring it explicit casts)

Does that sound good?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65552/new/

https://reviews.llvm.org/D65552