[Tablegen] Attempt to add support for patterns containing nodes with multiple results.

[craig.topper]

Summary

This patch attempts to add support nodes with multiple results. This is necessary to enable AVX512 masked scatter/gather support.

This change exposed some odd behavior with a few instructions in the R600 backend. Previously it was using IMPLICIT_DEF input to represent what was really a second output and this magically worked. But now that we used NumDefs directly this needs to be converted to a proper output. But to do that we need to be able to cast a second output type. So I've introduced a ValueTypeList that can contain multiple ValueTypes.

[hfinkel]

[+Tom, Matt for the R600 changes]

[• tstellarAMD]

The second output on those R600 instructions is just there to reserve a temp register to make it easier to lower the instruction later on. I think this patch should be OK.

[craig.topper]

Is there a more correct type than i1? I just picked i1 because that happened to be what got picked before due to some code that tries to fix a circular dependency in types. I don't think it was really intended to fire in this particular case but it did.

[craig.topper]

I had lost a change where I removed one of the other places where NumDefs was forced to be 1 if it was non-zero.

[hfinkel]

In D8373#142510, @tstellarAMD wrote:

The second output on those R600 instructions is just there to reserve a temp register to make it easier to lower the instruction later on. I think this patch should be OK.

Alright, I see what you mean, you have this:

def SI_INDIRECT_SRC : InstSI <
  (outs VGPR_32:$dst, SReg_64:$temp),
  (ins unknown:$src, VSrc_32:$idx, i32imm:$off),
  "si_indirect_src $dst, $temp, $src, $idx, $off",
  []
>;

and so when you use this:

// 1. Extract with offset
def : Pat<
  (vector_extract vt:$vec, (add i32:$idx, imm:$off)),
  (eltvt (SI_INDIRECT_SRC (IMPLICIT_DEF), $vec, $idx, imm:$off))
>;

the pattern generates the IMPLICIT_DEF as the output operand somehow (even though it is written as an input operand), and you get the scratch register that you need.

Would [Any, Any] make sense here too?

Craig, this is indeed a use case we need to support: having a pseudo-instruction with a 'scratch register' output definition. However, it does raise the question that, if we're going to support pattern matching instructions with multiple output operands (which is the point here), what should we do if the input pattern does not itself have multiple output operands?

In short, I don't understand your any_i1 solution. It does not help to match the type of vector_insert, etc., and SI_INDIRECT_SRC, etc. already have multiple output operands. The type of the second output operand might have an ambiguous type, but why does that matter if we're not matching against it? Forcing the user to choose a type that is sure to be ignored / irrelevant seems confusing.

[craig.topper]

But it's not really ignored. At the end of isel we have an SDNode with 2 results and those results need types.

Today that IMPLICIT_DEF got a type of i1 because this code triggered

if (!IterateInference && InferredAllPatternTypes &&
    !InferredAllResultTypes)
  IterateInference =
      ForceArbitraryInstResultType(Result.getTree(0), Result);

but based on the comment above that code I don't think this case was what that code was intended for. I guess this part of that comment applies for this case "output patterns should have register classes, not MVTs". Maybe I should force an arbitrary result type on extra results that don't show up in the original pattern?

[• tstellarAMD]

In D8373#142633, @hfinkel wrote:

In D8373#142510, @tstellarAMD wrote:

The second output on those R600 instructions is just there to reserve a temp register to make it easier to lower the instruction later on. I think this patch should be OK.

Alright, I see what you mean, you have this:

def SI_INDIRECT_SRC : InstSI <
  (outs VGPR_32:$dst, SReg_64:$temp),
  (ins unknown:$src, VSrc_32:$idx, i32imm:$off),
  "si_indirect_src $dst, $temp, $src, $idx, $off",
  []
>;

and so when you use this:

// 1. Extract with offset
def : Pat<
  (vector_extract vt:$vec, (add i32:$idx, imm:$off)),
  (eltvt (SI_INDIRECT_SRC (IMPLICIT_DEF), $vec, $idx, imm:$off))
>;

the pattern generates the IMPLICIT_DEF as the output operand somehow (even though it is written as an input operand), and you get the scratch register that you need.

Would [Any, Any] make sense here too?

Craig, this is indeed a use case we need to support: having a pseudo-instruction with a 'scratch register' output definition. However, it does raise the question that, if we're going to support pattern matching instructions with multiple output operands (which is the point here), what should we do if the input pattern does not itself have multiple output operands?

Hi Hal,

Do we need to support this use case because there are other targets that do this too? Otherwise if R600 is the only target doing this, would it help if I tried to rework this pseudo to only have one output?

In short, I don't understand your any_i1 solution. It does not help to match the type of vector_insert, etc., and SI_INDIRECT_SRC, etc. already have multiple output operands. The type of the second output operand might have an ambiguous type, but why does that matter if we're not matching against it? Forcing the user to choose a type that is sure to be ignored / irrelevant seems confusing.

[hfinkel]

In D8373#142779, @craig.topper wrote:

But it's not really ignored. At the end of isel we have an SDNode with 2 results and those results need types.

Today that IMPLICIT_DEF got a type of i1 because this code triggered

if (!IterateInference && InferredAllPatternTypes &&
    !InferredAllResultTypes)
  IterateInference =
      ForceArbitraryInstResultType(Result.getTree(0), Result);

but based on the comment above that code I don't think this case was what that code was intended for. I guess this part of that comment applies for this case "output patterns should have register classes, not MVTs". Maybe I should force an arbitrary result type on extra results that don't show up in the original pattern?

Yes, I actually think that would be better. While we do need a type at the SDAG level, all that really matters in the end is that we end up with a register from the correct register class. There's no need for the user to pick a type for a result that is not really used aside from its affect during register allocation.

[hfinkel]

In D8373#142801, @tstellarAMD wrote:

In D8373#142633, @hfinkel wrote:

In D8373#142510, @tstellarAMD wrote:

The second output on those R600 instructions is just there to reserve a temp register to make it easier to lower the instruction later on. I think this patch should be OK.

...

Craig, this is indeed a use case we need to support: having a pseudo-instruction with a 'scratch register' output definition. However, it does raise the question that, if we're going to support pattern matching instructions with multiple output operands (which is the point here), what should we do if the input pattern does not itself have multiple output operands?

Hi Hal,

Do we need to support this use case because there are other targets that do this too? Otherwise if R600 is the only target doing this, would it help if I tried to rework this pseudo to only have one output?

I am fairly certain that you're not the only target doing this. I recall doing this myself at some point (although I selected the instructions manually in *ISelDAGToDAG because I did not discover the same trick that you did). Having this supported by TableGen properly will be convenient.

In short, I don't understand your any_i1 solution. It does not help to match the type of vector_insert, etc., and SI_INDIRECT_SRC, etc. already have multiple output operands. The type of the second output operand might have an ambiguous type, but why does that matter if we're not matching against it? Forcing the user to choose a type that is sure to be ignored / irrelevant seems confusing.

[craig.topper]

It seems with some other change that I made after adding the typecast in R600 the need for typecast went away. Yay!

[hfinkel]

LGTM, thanks!

[craig.topper]

Committed in r232798.

lib/Target/R600/SIInstructions.td

 /********** ====================== **********/
 /**********   Indirect adressing   **********/
 /********** ====================== **********/
 
 multiclass SI_INDIRECT_Pattern <ValueType vt, ValueType eltvt, SI_INDIRECT_DST IndDst> {
 
   // 1. Extract with offset
   def : Pat<
-    (vector_extract vt:$vec, (add i32:$idx, imm:$off)),
-    (eltvt (SI_INDIRECT_SRC (IMPLICIT_DEF), $vec, $idx, imm:$off))
+    (eltvt (vector_extract vt:$vec, (add i32:$idx, imm:$off))),
+    (SI_INDIRECT_SRC $vec, $idx, imm:$off)
   >;
 
   // 2. Extract without offset
   def : Pat<
-    (vector_extract vt:$vec, i32:$idx),
-    (eltvt (SI_INDIRECT_SRC (IMPLICIT_DEF), $vec, $idx, 0))
+    (eltvt (vector_extract vt:$vec, i32:$idx)),
+    (SI_INDIRECT_SRC $vec, $idx, 0)
   >;
 
   // 3. Insert with offset
   def : Pat<
     (vector_insert vt:$vec, eltvt:$val, (add i32:$idx, imm:$off)),
-    (IndDst (IMPLICIT_DEF), $vec, $idx, imm:$off, $val)
+    (IndDst $vec, $idx, imm:$off, $val)
   >;
 
   // 4. Insert without offset
   def : Pat<
     (vector_insert vt:$vec, eltvt:$val, i32:$idx),
-    (IndDst (IMPLICIT_DEF), $vec, $idx, 0, $val)
+    (IndDst $vec, $idx, 0, $val)
   >;
 }
 
 defm : SI_INDIRECT_Pattern <v2f32, f32, SI_INDIRECT_DST_V2>;
 defm : SI_INDIRECT_Pattern <v4f32, f32, SI_INDIRECT_DST_V4>;
 defm : SI_INDIRECT_Pattern <v8f32, f32, SI_INDIRECT_DST_V8>;
 defm : SI_INDIRECT_Pattern <v16f32, f32, SI_INDIRECT_DST_V16>;
 

utils/TableGen/CodeGenDAGPatterns.cpp

if (Tree)
         Op = DI->getDef();
     assert(Op && "Invalid Fragment");
     return GetNumNodeResults(Op, CDP);
   }
 
   if (Operator->isSubClassOf("Instruction")) {
     CodeGenInstruction &InstInfo = CDP.getTargetInfo().getInstruction(Operator);
 
-    // FIXME: Should allow access to all the results here.
-    unsigned NumDefsToAdd = InstInfo.Operands.NumDefs ? 1 : 0;
+    unsigned NumDefsToAdd = InstInfo.Operands.NumDefs;
+
+    // Subtract any defaulted outputs.
+    for (unsigned i = 0; i != InstInfo.Operands.NumDefs; ++i) {
+      Record *OperandNode = InstInfo.Operands[i].Rec;
+
+      if (OperandNode->isSubClassOf("OperandWithDefaultOps") &&
+          !CDP.getDefaultOperand(OperandNode).DefaultOps.empty())
+        --NumDefsToAdd;
+    }
 
     // Add on one implicit def if it has a resolvable type.
     if (InstInfo.HasOneImplicitDefWithKnownVT(CDP.getTargetInfo()) !=MVT::Other)
       ++NumDefsToAdd;
     return NumDefsToAdd;
   }
 
   if (Operator->isSubClassOf("SDNodeXForm"))
if (getOperator()->isSubClassOf("Instruction")) {
     const DAGInstruction &Inst = CDP.getInstruction(getOperator());
     CodeGenInstruction &InstInfo =
       CDP.getTargetInfo().getInstruction(getOperator());
 
     bool MadeChange = false;
 
     // Apply the result types to the node, these come from the things in the
     // (outs) list of the instruction.
-    // FIXME: Cap at one result so far.
-    unsigned NumResultsToAdd = InstInfo.Operands.NumDefs ? 1 : 0;
+    unsigned NumResultsToAdd = std::min(InstInfo.Operands.NumDefs,
+                                        Inst.getNumResults());
     for (unsigned ResNo = 0; ResNo != NumResultsToAdd; ++ResNo)
       MadeChange |= UpdateNodeTypeFromInst(ResNo, Inst.getResult(ResNo), TP);
 
     // If the instruction has implicit defs, we apply the first one as a result.
     // FIXME: This sucks, it should apply all implicit defs.
     if (!InstInfo.ImplicitDefs.empty()) {
       unsigned ResNo = NumResultsToAdd;
 
const DAGInstruction &CodeGenDAGPatterns::parseInstructionPattern(
   // added to the machine instruction the node corresponds to.
   unsigned NumResults = InstResults.size();
 
   // Parse the operands list from the (ops) list, validating it.
   assert(I->getArgList().empty() && "Args list should still be empty here!");
 
   // Check that all of the results occur first in the list.
   std::vector<Record*> Results;
-  TreePatternNode *Res0Node = nullptr;
+  SmallVector<TreePatternNode *, 2> ResNodes;
   for (unsigned i = 0; i != NumResults; ++i) {
     if (i == CGI.Operands.size())
       I->error("'" + InstResults.begin()->first +
                "' set but does not appear in operand list!");
     const std::string &OpName = CGI.Operands[i].Name;
 
     // Check that it exists in InstResults.
     TreePatternNode *RNode = InstResults[OpName];
     if (!RNode)
       I->error("Operand $" + OpName + " does not exist in operand list!");
 
-    if (i == 0)
-      Res0Node = RNode;
+    ResNodes.push_back(RNode);
+
     Record *R = cast<DefInit>(RNode->getLeafValue())->getDef();
     if (!R)
       I->error("Operand $" + OpName + " should be a set destination: all "
                "outputs must occur before inputs in operand list!");
 
     if (!checkOperandClass(CGI.Operands[i], R))
       I->error("Operand $" + OpName + " class mismatch!");
 
const DAGInstruction &CodeGenDAGPatterns::parseInstructionPattern(
 
   if (!InstInputsCheck.empty())
     I->error("Input operand $" + InstInputsCheck.begin()->first +
              " occurs in pattern but not in operands list!");
 
   TreePatternNode *ResultPattern =
     new TreePatternNode(I->getRecord(), ResultNodeOperands,
                         GetNumNodeResults(I->getRecord(), *this));
-  // Copy fully inferred output node type to instruction result pattern.
-  for (unsigned i = 0; i != NumResults; ++i)
-    ResultPattern->setType(i, Res0Node->getExtType(i));
+  // Copy fully inferred output node types to instruction result pattern.
+  for (unsigned i = 0; i != NumResults; ++i) {
+    assert(ResNodes[i]->getNumTypes() == 1 && "FIXME: Unhandled");
+    ResultPattern->setType(i, ResNodes[i]->getExtType(0));
+  }
 
   // Create and insert the instruction.
   // FIXME: InstImpResults should not be part of DAGInstruction.
   DAGInstruction TheInst(I, Results, Operands, InstImpResults);
   DAGInsts.insert(std::make_pair(I->getRecord(), TheInst));
 
   // Use a temporary tree pattern to infer all types and make sure that the
   // constructed result is correct.  This depends on the instruction already
for (unsigned i = 0, e = Instrs.size(); i != e; ++i) {
     // null_frag.
     if (!LI || LI->getSize() == 0 || hasNullFragReference(LI)) {
       std::vector<Record*> Results;
       std::vector<Record*> Operands;
 
       CodeGenInstruction &InstInfo = Target.getInstruction(Instrs[i]);
 
       if (InstInfo.Operands.size() != 0) {
-        if (InstInfo.Operands.NumDefs == 0) {
-          // These produce no results
-          for (unsigned j = 0, e = InstInfo.Operands.size(); j < e; ++j)
-            Operands.push_back(InstInfo.Operands[j].Rec);
-        } else {
-          // Assume the first operand is the result.
-          Results.push_back(InstInfo.Operands[0].Rec);
+        for (unsigned j = 0, e = InstInfo.Operands.NumDefs; j < e; ++j)
+          Results.push_back(InstInfo.Operands[j].Rec);
 
-          // The rest are inputs.
-          for (unsigned j = 1, e = InstInfo.Operands.size(); j < e; ++j)
-            Operands.push_back(InstInfo.Operands[j].Rec);
-        }
-      }
+        for (unsigned j = InstInfo.Operands.NumDefs,
+               e = InstInfo.Operands.size(); j < e; ++j)
 
       // Create and insert the instruction.
       std::vector<Record*> ImpResults;
       Instructions.insert(std::make_pair(Instrs[i],
                           DAGInstruction(nullptr, Results, Operands, ImpResults)));
       continue;  // no pattern.
     }