[llvm] r306875 - GlobalISel: add G_IMPLICIT_DEF instruction.

[Quentin Colombet via llvm-commits]

Hi Tim,

> On Jun 30, 2017, at 4:07 PM, Tim Northover <tnorthover at apple.com> wrote:
> 
>> I agree that enforcing type-consistency is not an implementation detail. What I am saying is that it does not need to be tight to new G_ opcodes. Again, what can we do with that additional opcode that we couldn’t without.
> 
> Have neat code. Of course we could hack away at the MachineVerifier, Legalizer and whatever else to explain that, no COPY and PHI and IMPLICIT_DEF are special and have this dual role. But I think having G_* variants makes the requirements on each clearer.

Clearer yes (at least in some cases), more practical I am not sure. That’s why we are debating :).

> 
>>> The other is resolved by the legalizer[*] but a question relevant in any stage before that too.
>>> 
>>>> In particular, should G_COPY returns true to MachineInstr::isCopy, ditto for G_IMPLICIT_DEF with MachineInstr::isImplicitDef?
>>> 
>>> I'm not sure it matters until we start dealing with combines properly. And even then G_* is going to be entirely contained in GISel code so we can pick whichever is more convenient for us at the time.
>> 
>> Well, that’s already a problem if you want to use for instance getFirstNonPHI. I.e., GISel specific is going to “leak" outside of GISel domain per say.
> 
> I'm afraid I don't get this. You mean that even after GIsel these functions will be checking for G_* nodes that won't exist? I'm not entirely sure that's a problem, but if it is then we have a clearly defined line before which instructions will be G_* and after which they'll be * (InstructionSelect) so we can add another callback (getFirstNonGenericPHI or something) and use it in the right places.

I mean, right now, if you use getFirstNonPHI, the code will not work as expected with G_PHI. Like, you may end up inserting a regular instruction before a G_PHI, which is probably not what you would expect. If you teach that API to also deal with G_PHI, then G_PHI is not internal to GISel anymore. I’m nitpicking here, but that’s the thing that I am afraid of: we may taint a lot of code and doesn’t seem like a great design.

If we go with two different APIs it’s probably even worse. All the code that works on GISel must use the right one, which is fine, but if we happen to have code that works before ISel and after ISel it has to check in which mode it has to run to call the right one. That kind of code does not exists yet, but we can’t rule out it will never exist.

One of the property of GISel was that targets could plug target specific pass wherever in the pipeline without to worry about GISel specifics. This goes against that because we will have to use different APIs to interact with the IR.

> 
>>> [*] Though even then maybe only because we're being a bit naive right now. When 512-bit types are flying around more regularly because QQQQ is a valid type for stores RegBankSelect and ISel might end up getting in on the fun too.
>> 
>> I didn’t get that comment.
> 
> Since some NEON stores handle up to 512 bits, once things are fully implemented we're likely to see things like this after legalization:
> 
>    %0(s512) = G_LOAD %ptr
>    %1(s512) = (G_)?COPY %0
> 
> The optimum mapping for this is almost certainly %0(QQQQ), %1(Q,Q,Q,Q) and after InstructionSelection this would have to become something like
> 
>    %1.0 = COPY %0:qsub0
>    %1.1 = COPY %0:qsub1
>    %1.2 = COPY %0:qsub2
>    %1.3 = COPY %0:qsub3.

No, that should be:
%1 = COPY %0
Otherwise you need to have something like as input of ISel:
%1.0 = EXTRACT_SUBREG %0, qsub0
%1.1 = EXTRACT_SUBREG %0, qsub0
Etc.

What I am saying is if the better mapping is the extract_subreg sequence, then regbankselect would have to do the expansion. So (G_?)COPYs that hit ISel are really copies.

Anyhow, my biggest concern with those different but somehow identical opcodes is that it may be clearer on one side (GISel specific stuff), but more obscure on another (things that lie in between).
Let me take an example.

One of the goal of GISel is to be able to use target specific construction whenever we want, in particular, when lowering an intrinsic we can directly use the related instruction.
Let say we have:
G_INSTRINC <ID:BLA>, vreg1(s32), vreg2(s32)

Then, should the lowering looks like:
regA(regclassA) = COPY <== should those be COPY or G_COPY?
regB(regclassB) = COPY <== if we go with G_COPY that means I need to set a meaningful type, right?
BLA <implicit-def>

At the end of the day, I really think this is all implementation details as I think any approach would work. However I think we don’t know in the long run what is the best approach (in particular with the tainting aspect of the new opcodes on the non-GISel code). What I know though is if we don’t have a clear model that we can enforce at the API level and with the verifier, we will have trouble down the road. So we need to settle all that stuff before moving forward.

To summarize we need to agree on a design for those opcodes before adding more.

Assuming with go with the G_* approach, we probably want to have GISel not being an optional thing anymore, so we can start “tainting” the existing APIs without spread ifdefs all around.

Cheers,
-Quentin

PS: I don’t want you to think I am picking on you, I really want we end up with the best design and I certainly don’t pretend to know what it looks like. Thus, I am looking for a broader discussion and a consensus in the end.

> 
> Cheers.
> 
> Tim.