[PATCH] D22373: [GlobalISel] Introduce a simple instruction selector.

[Matthias Braun via llvm-commits]

> On Jul 14, 2016, at 3:00 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Hi Matthias,
> 
>> On Jul 14, 2016, at 11:35 AM, Matthias Braun <matze at braunis.de> wrote:
>> 
>> MatzeB added a comment.
>> 
>> I just looked at the MachineRegisterInfo parts. This is also a highlevel comment, not necessarily about this patch (which just continues the current direction): It seems to me like generic regs and "normal" vregs don't have that much in common, they mostly share the same kind of MachineOperand and the same def/use lists. Apart from that they contain different information (register bank + size vs. register class). Wouldn't it make sense to introduce a new MachineOperand kind for them and separate them from "normal" vregs?
> 
> What kind of gain do you see by having a new kind of MachineOperand?
Clarity and reduced complexity!

There basically was a new kind of registers invented without adding new infrastructure for it. So we end up with strange functions like getRegClassOrNull(), getRegClass(), getRegBankOrNull(), getRegClassOrRegBank() which IMO just enables confusing code where those two sort of register needlessly mixed. You also have to be aware what sort of vreg you use now, and there is a bunch of methods that will probably fail with "generic" vregs (constrainRegClass(), recomputeRegClass(), addLiveIn(vreg=)) without that being too obvious (they all take a vreg operand).

> From my point of view, this is not needed. Yes, register banks are a higher abstraction than register classes, but other than that they are the same kind of registers. The size thing is just encoded in the register class, but technically, you could ask for the size of a “normal” vreg.
> 
> The bottom line is, the way we deal with both is identical except from the register bank/class field.
Well all the data associated with a vreg is the register class.
All the data associated with a "generic" vreg is the register bank/size. So you exchanged all the data associated with it, that does not seem too identical to me.

> 
> Ultimately, all virtual registers would have a register bank (dynamically deduce from there current register class). Right now this is not the case because RegisterBanks are in a different, optional, library.

Additions in this commit like the constrainRegClass() indicate to me that we are mixing too much stuff into the low-level features instead of cleanly introducing a new concept that builds on top or orthogonal to the existing stuff.

Just wanted to point out that I feel there may be a better design, the current one certainly works.

- Matthias