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

Thu Jul 14 16:57:15 PDT 2016

> On Jul 14, 2016, at 4:09 PM, Matthias Braun <matze at braunis.de> wrote:
> 
>> 
>> On Jul 14, 2016, at 3:55 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>> 
>>> 
>>> On Jul 14, 2016, at 3:21 PM, Matthias Braun <matze at braunis.de <mailto:matze at braunis.de>> wrote:
>>> 
>>> 
>>>> On Jul 14, 2016, at 3:00 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>> 
>>>> Hi Matthias,
>>>> 
>>>>> On Jul 14, 2016, at 11:35 AM, Matthias Braun <matze at braunis.de <mailto: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!
>> 
>> So far with the proper APIs, I do not see the complexity, but maybe I am too much into it.
>> As for clarity, I guess we could say that we lower the representation of the virtual register and in that case, this is more that one new kind that we need. Also why would we make a different kind for generic vs normal reg, but not from normal to phys.
>> 
>> What I would suggest instead, if we really want to improve clarity, is to rework MRI not MachineOperand. Like a concept of register and then several variants:
>> - phys
>> - virtual
>>  * fully generic (sized)
>>  * mapped generic (sized + register bank)
>>  * constrained (our current virtual: register class (from which you can deduce the size and register bank))
> 
> true, we can do the differentiation at the MRI level.
> 
>> 
>>> 
>>> 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.
>> 
>> I agree those can be confusing, but not that much different from phys vs normal register I would say. We add new types like I said. I am not opposed to a clean-up on the contrary but MachineOperand is not the right spot for that IMO.
>> 
>>> 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).
>> 
>> The plan is to have those methods to work with the register bank concept. The problem is that we can’t do it properly today because that would create a dependency on the “optional” library. Technically a register bank is just a super register class, or a register class without encoding constraints.
> 
> Maybe the problem lies here and we should rather introduce a set of new methods instead of re-using the ones meant for virtual registers?

That would work for me.

> 
>> 
>>> 
>>>> 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.
>> 
>> That's not different than phys vs. Normal. E.g., the way to get a regclass for phys reg is confusing compared to vreg.
> True, it's just a different concept, however if a method has a "unsigned vreg" parameter I currently don't know which it is, it may already be enough to have some consistent style here.

I am not sure I get what you mean by consistent style.

> 
>> 
>>> 
>>>> 
>>>> 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.
>> 
>> Having constrainRegClass working with RegisterBank looks like the right call to me and that looks to be built on top of the current concept. I miss the problem for that particular example.
>> 
>> Don’t take me wrong. I believe we could do a lot of clean-ups in the Machine related APIs. I however believe that the right time to do it is when we will productize GISel, which is not what we do now :). Indeed, I believe we need to see all the problems first before we decide on the best design.
>> 
>> Anyhow, thanks for bring the problem.
> 
> What about a new constrainGenericReg() method so it is obvious with what register sort we are dealing with and we don't add extra code paths to the existing function that deals with vregs.

That would work for me.

> 
> - Matthias

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160714/8cad08a1/attachment.html>