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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

> SGTM, will do. While I'm there, anything else?


I'd move constrainSelectedInstRegOperands in the based class (as a static) to have it available for all the targets.

Other than that LGTM, but please address the other comments first.

Cheers,
-Quentin


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:60
@@ -59,3 +59,3 @@
     return NewRC;
   if (NewRC->getNumRegs() < MinNumRegs)
     return nullptr;
----------------
ab wrote:
> qcolombet wrote:
> > Actually what I had in mind was to get the size from the type with, like you said, querying the MI for the def size (via getUniqueDef). However, we would still need to access this information from the vreg.
> > As having the type on the vreg, no, that does not work. Vregs can change type based on their uses (like and s64, add <2 x s32>).
> > 
> > But yeah, long term, this code shouldn't be needed. Add a comment about that?
> > Actually what I had in mind was to get the size from the type with, like you said, querying the MI for the def size (via getUniqueDef). However, we would still need to access this information from the vreg.
> > As having the type on the vreg, no, that does not work. Vregs can change type based on their uses (like and s64, add <2 x s32>).
> 
> Right;  I suppose you could see a vreg as having an "initial" type, the one on the def.   The type(s) of an MI is then just the type(s) of its def(s).  I admit it's also somewhat awkward, but at least avoids duplicating the info in a separate, seldom accessed, data structure.  Thoughts?
> 
> > But yeah, long term, this code shouldn't be needed. Add a comment about that?
> 
> SGTM, will do.  While I'm there, anything else?
>  at least avoids duplicating the info in a separate [...] data structure.

Right now, yes, this is duplicated, but with what I had in mind, this is an indirect access: vreg -> getDef -> getType -> getSize. That's not pretty, but that works.

> The type(s) of an MI is then just the type(s) of its def(s)

Honestly, the main reason why I discarded this design was because I thought it would have been confusing to ask for the type of some operand and get something not related to the operand. More over, the concept of having typed register is strange to me.
The bottom line is that it may indeed, be more efficient (fewer indirections), but in terms of concept I don't like it. If we manage to spin that in a reasonable manner, that would work.

Anyhow, this is something we need to clean-up at some point (do we actually need the size in the end?), but for now, let us stick on that.


https://reviews.llvm.org/D22373