Page MenuHomePhabricator

TableGen: Fix logic for default operands
ClosedPublic

Authored by arsenm on Feb 18 2020, 3:50 PM.

Details

Summary

This was checking for default operands in the current DAG instruction,
rather than the correct result operand list. I'm not entirely sure how
this managed to work before, but was failing for me when multiple
default operands were overridden.

Diff Detail

Event Timeline

arsenm created this revision.Feb 18 2020, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2020, 3:50 PM
Herald added a subscriber: wdng. · View Herald Transcript

I might as well admit my ignorance up front: despite having been the last person to touch this code, I don't really understand what the distinction is between a CodeGenInstruction and a DAGInstruction here, apart from the fact that one of them has an operand list that includes the outputs as well as the inputs.

As far as I can see, each DAGInstruction is built from a CodeGenInstruction by the loop in CodeGenDAGPatterns::ParseInstructions(), and it looks to me from reading that code as if the number of operands of the DAGInstruction should always be the same as NumFixedOperands - NumResults for the corresponding CodeGenInstruction, and the operand list is a copy of the same list of records. So without a test case or an existing example, I can't see how this change is making any difference at all.

I wrote this default-operand checking to use with the MVE instructions in the Arm backend. Certainly in that context it seems to make no difference which of these pieces of code is used: an instruction like MVE_VADDi8, for example, has three input operands in both of these views, with the last one being defaultable (even though it then consists of multiple sub-operands that show up as separate children of the DAG node).

Do you have an example of a case in which the code you're replacing gives different answers before and after this patch?

I might as well admit my ignorance up front: despite having been the last person to touch this code, I don't really understand what the distinction is between a CodeGenInstruction and a DAGInstruction here, apart from the fact that one of them has an operand list that includes the outputs as well as the inputs.

As far as I can see, each DAGInstruction is built from a CodeGenInstruction by the loop in CodeGenDAGPatterns::ParseInstructions(), and it looks to me from reading that code as if the number of operands of the DAGInstruction should always be the same as NumFixedOperands - NumResults for the corresponding CodeGenInstruction, and the operand list is a copy of the same list of records. So without a test case or an existing example, I can't see how this change is making any difference at all.

The DAGInstruction is the pattern instruction, which may be missing the defaulted operands in a source pattern. The CodeGenInstruction is the target MachineInstr concept.

I wrote this default-operand checking to use with the MVE instructions in the Arm backend. Certainly in that context it seems to make no difference which of these pieces of code is used: an instruction like MVE_VADDi8, for example, has three input operands in both of these views, with the last one being defaultable (even though it then consists of multiple sub-operands that show up as separate children of the DAG node).

Do you have an example of a case in which the code you're replacing gives different answers before and after this patch?

I get incorrect type inference failures in D74832 without this patch since it ends up trying to merge the type of a non-default operand in with a default

simon_tatham accepted this revision.Feb 19 2020, 8:38 AM

I get incorrect type inference failures in D74832 without this patch since it ends up trying to merge the type of a non-default operand in with a default

I'm completely confused by that patch: I tried to apply it locally in order to see your inference failure for myself, but I couldn't, because of conflicts. For example, it removes a declaration of selectVOP3PMods0 from AMDGPUInstructionSelector.h, but as far as git log can tell me, that declaration has never existed in that header file at all, as of current master.

The DAGInstruction is the pattern instruction, which may be missing the defaulted operands in a source pattern. The CodeGenInstruction is the target MachineInstr concept.

OK, thanks; now I see what I missed there – CodeGen::parseInstructionPattern can also create instances of DAGInstruction, and it creates the interesting ones, whereas ParseInstructions() just makes the trivial ones. But parseInstructionPattern does the creation using an emplace, without mentioning the class name, so my crude grep didn't find it...

Now I understand how this didn't cause a problem in MVE, because in the MVE instruction definitions, we generally didn't bother filling in the pattern fields in the Instruction records themselves. All the instructions with a defaulted operand are predicated, so they need more than one pattern anyway for unpredicated and predicated instructions, so it was easier to put them all in separate Pat records. So that's why the operand lists in Inst and InstInfo look identical in our case.

In that case, this patch looks sensible to me, although I have just proved that I'm a long way from being an expert in this area!

This revision is now accepted and ready to land.Feb 19 2020, 8:38 AM
dsanders accepted this revision.Feb 19 2020, 12:08 PM

I haven't dug very deep into this but it makes sense to me to be reading the InstInfo and as Simon notes, they should be identical for the cases that already work. On that basis LGTM

Could you add a small test case to the TableGen directory? We don't have much information about how default operands behave and a couple easy to find examples would help with that.

You might need to do something similar for GlobalISel as there was a lot of guesswork in the implementation of default operands there.

I haven't dug very deep into this but it makes sense to me to be reading the InstInfo and as Simon notes, they should be identical for the cases that already work. On that basis LGTM

Could you add a small test case to the TableGen directory? We don't have much information about how default operands behave and a couple easy to find examples would help with that.

You might need to do something similar for GlobalISel as there was a lot of guesswork in the implementation of default operands there.

I already fixed the handling in GlobalISelEmitter a while ago

I haven't dug very deep into this but it makes sense to me to be reading the InstInfo and as Simon notes, they should be identical for the cases that already work. On that basis LGTM

Could you add a small test case to the TableGen directory? We don't have much information about how default operands behave and a couple easy to find examples would help with that.

Oddly this seems to only break if the same instruction is used in two different pattern contexts, one using the override and one not. I would expect these to be independent