TableGen: Fix logic for default operands

[arsenm]

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.

[simon_tatham]

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?

[arsenm]

In D74803#1882699, @simon_tatham wrote:

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]

In D74803#1882722, @arsenm wrote:

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!

[dsanders]

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.

[arsenm]

In D74803#1883340, @dsanders wrote:

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

[arsenm]

In D74803#1883340, @dsanders wrote:

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

[arsenm]

de6e968c0d44d21b16da9b6ec07599e529b8482a

llvm/utils/TableGen/CodeGenDAGPatterns.cpp

if (getOperator()->getName() == "INSERT_SUBREG") {
         if (!isOperandClass(SubIdxChild, "SubRegIndex")) {
           TP.error("REG_SEQUENCE requires a SubRegIndex for operand " +
                    Twine(I + 1) + "!");
           return false;
         }
       }
     }
 
+    unsigned NumResults = Inst.getNumResults();
+    unsigned NumFixedOperands = InstInfo.Operands.size();
+
     // If one or more operands with a default value appear at the end of the
     // formal operand list for an instruction, we allow them to be overridden
     // by optional operands provided in the pattern.
     //
     // But if an operand B without a default appears at any point after an
     // operand A with a default, then we don't allow A to be overridden,
     // because there would be no way to specify whether the next operand in
     // the pattern was intended to override A or skip it.
-    unsigned NonOverridableOperands = Inst.getNumOperands();
-    while (NonOverridableOperands > 0 &&
-           CDP.operandHasDefault(Inst.getOperand(NonOverridableOperands-1)))
+    unsigned NonOverridableOperands = NumFixedOperands;
+    while (NonOverridableOperands > NumResults &&
+           CDP.operandHasDefault(InstInfo.Operands[NonOverridableOperands-1].Rec))
       --NonOverridableOperands;
 
     unsigned ChildNo = 0;
-    for (unsigned i = 0, e = Inst.getNumOperands(); i != e; ++i) {
-      Record *OperandNode = Inst.getOperand(i);
+    assert(NumResults <= NumFixedOperands);
+    for (unsigned i = NumResults, e = NumFixedOperands; i != e; ++i) {
+      Record *OperandNode = InstInfo.Operands[i].Rec;
 
       // If the operand has a default value, do we use it? We must use the
       // default if we've run out of children of the pattern DAG to consume,
       // or if the operand is followed by a non-defaulted one.
       if (CDP.operandHasDefault(OperandNode) &&
           (i < NonOverridableOperands || ChildNo >= getNumChildren()))
         continue;