TableGen/GlobalISel: Allow output instructions with multiple defs

[arsenm]

Summary

The DAG behavior allows matchching input patterns with a single result
to the first result of an output instruction that defines multiple
results. The remaining defs are implicitly dead.

This starts to fix using manual selection for AMDGPU add/sub (although
it's still needed, mostly because it's also still needed for
G_PTR_ADD).

[paquette]

llvm/utils/TableGen/GlobalISelEmitter.cpp

4809–4811

This message is misleading with this change

[arsenm]

Fix error message. I'll probably be removing this check soon, since I have cases that check use_empty on the results and discard the source result

[madhur13490]

llvm/test/TableGen/GlobalISelEmitter-output-discard.td

2

You many want to check with -optimize-match-table=true and false too.

[foad]

llvm/utils/TableGen/GlobalISelEmitter.cpp

4352–4353

"results after the first is" is bad English. How about "any result after the first is" ?

[madhur13490]

llvm/utils/TableGen/GlobalISelEmitter.cpp

4809

I was thinking about this for the other change. If DstI's NumDefs are less than what source pattern is defining then we can ignore the defs from source pattern. So, this error message shouldn't be there and may be we can emit a warning or something. OTOH, if it's opposite, i.e. source pattern is defining a fewer values then dstI's some value will be undefined. In any case, there need not be an error because both of these are valid some or the other backend. I am not sure what we should be doing ideally from matching perspective but both cases can occur.

[paquette]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2613–2619

Are we going to need more variations on RegState in the future?

If so, maybe it'd be better to do something like the pseudocode below?

RegState = nothing
if (IsDef) {
   RegState = "RegState::Define";
   if (IsDead)
      RegState += "|RegState::Dead";
}

if (RegState != nothing)
  Table << MatchTable::NamedValue(RegState);
else
   Table << MatchTable::IntValue(0);

4351

Could you get rid of the if in the loop by doing something like this?

unsigned NumDefs = DstI->Operands.NumDefs;
if (!NumDefs)
  return InsertPt;

const CodeGenInstruction *DstI = DstMIBuilder.getCGI();
DstMIBuilder.addRenderer<CopyRenderer>(DstI->Operands[0].Name);

for (unsigned I = 1; I < NumDefs; ++I) {
  const TypeSetByHwMode &ExtTy = Dst->getExtType(I);
  ...
}

return InsertPt;

[arsenm]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2613–2619

I don't think so. I think implicit is the only other one that might make sense, but I don't think there's any contexts where the selector would need implicit virtual registers

4809

I think a warning would be counterproductive. TableGen doesn't really emit warnings. We could maybe invent some new construct to indicate explicitly discarded results, but I'm not sure what it would look like

[arsenm]

Address comments

[arsenm]

llvm/test/TableGen/GlobalISelEmitter-output-discard.td

2

We're not really optimizing this and adding a second set of checks increases the maintenance cost of the test

[arsenm]

ping

[arsenm]

ping

[paquette]

LGTM

[arsenm]

ee3feef5aaaa3c385fbe08bdb2d48829ad440b56

llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-add.mir

 
 body: |
   bb.0:
     liveins: $vgpr0
 
     ; GFX6-LABEL: name: add_neg_inline_const_64_to_sub_s32_v
     ; GFX6: liveins: $vgpr0
     ; GFX6: [[COPY:%[0-9]+]]:vgpr_32 = COPY $vgpr0
-    ; GFX6: [[V_MOV_B32_e32_:%[0-9]+]]:vgpr_32 = V_MOV_B32_e32 4294967232, implicit $exec
-    ; GFX6: %2:vgpr_32, dead %3:sreg_64_xexec = V_ADD_I32_e64 [[COPY]], [[V_MOV_B32_e32_]], 0, implicit $exec
+    ; GFX6: %2:vgpr_32, dead %3:sreg_64 = V_SUB_I32_e64 [[COPY]], 64, 0, implicit $exec
     ; GFX6: S_ENDPGM 0, implicit %2
     ; GFX9-LABEL: name: add_neg_inline_const_64_to_sub_s32_v
     ; GFX9: liveins: $vgpr0
     ; GFX9: [[COPY:%[0-9]+]]:vgpr_32 = COPY $vgpr0
     ; GFX9: [[V_SUB_U32_e64_:%[0-9]+]]:vgpr_32 = V_SUB_U32_e64 [[COPY]], 64, 0, implicit $exec
     ; GFX9: S_ENDPGM 0, implicit [[V_SUB_U32_e64_]]
     %0:vgpr(s32) = COPY $vgpr0
     %1:vgpr(s32) = G_CONSTANT i32 -64

llvm/test/TableGen/Common/GlobalISelEmitterCommon.td

 // Common target boilerplate for GlobalISelEmitter tests.
 def MyTargetISA : InstrInfo;
 def MyTarget : Target { let InstructionSet = MyTargetISA; }
 
 class MyTargetGenericInstruction : GenericInstruction {
   let Namespace = "MyTarget";
 }
 
 def R0 : Register<"r0"> { let Namespace = "MyTarget"; }
 def GPR32 : RegisterClass<"MyTarget", [i32], 32, (add R0)>;
 def GPR32Op : RegisterOperand<GPR32>;
 def F0 : Register<"f0"> { let Namespace = "MyTarget"; }
 def FPR32 : RegisterClass<"MyTarget", [f32], 32, (add F0)>;
 def FPR32Op : RegisterOperand<FPR32>;
+def B0 : Register<"b0"> { let Namespace = "MyTarget"; }
+def GPR8 : RegisterClass<"MyTarget", [i8], 8, (add B0)>;
+
 def p0 : PtrValueType <i32, 0>;
 
 class I<dag OOps, dag IOps, list<dag> Pat>
   : Instruction {
   let Namespace = "MyTarget";
   let OutOperandList = OOps;
   let InOperandList = IOps;
   let Pattern = Pat;
 }

llvm/test/TableGen/GlobalISelEmitter-output-discard.td

+// RUN: llvm-tblgen -gen-global-isel -warn-on-skipped-patterns -I %p/../../include -I %p/Common %s -o - < %s | FileCheck -check-prefix=GISEL %s
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+// Test that extra explicit results are treated as dead defs.
+def ADD_CO : I<(outs GPR32:$dst, GPR8:$flag),
+               (ins GPR32Op:$src0, GPR32Op:$src1), []>;
+
+// GISEL: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_ADD,
+// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s32,
+// GISEL-NEXT: GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
+// GISEL-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// GISEL-NEXT: // (add:{ *:[i32] } i32:{ *:[i32] }:$src0, i32:{ *:[i32] }:$src1)  =>  (ADD_CO:{ *:[i32] }:{ *:[i8] } GPR32:{ *:[i32] }:$src0, GPR32:{ *:[i32] }:$src1)
+// GISEL-NEXT: GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s8,
+// GISEL-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::ADD_CO,
+// GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// GISEL-NEXT: GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/RegState::Define|RegState::Dead,
+// GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src0
+// GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/2, // src1
+// GISEL-NEXT: GIR_EraseFromParent, /*InsnID*/0,
+// GISEL-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+def : Pat <
+  (add i32:$src0, i32:$src1),
+  (ADD_CO GPR32:$src0, GPR32:$src1)
+>;

llvm/utils/TableGen/GlobalISelEmitter.cpp

 /// This is used to chain instructions together when emitting multiple
 /// instructions.
 class TempRegRenderer : public OperandRenderer {
 protected:
   unsigned InsnID;
   unsigned TempRegID;
   const CodeGenSubRegIndex *SubRegIdx;
   bool IsDef;
+  bool IsDead;
 
 public:
   TempRegRenderer(unsigned InsnID, unsigned TempRegID, bool IsDef = false,
-                  const CodeGenSubRegIndex *SubReg = nullptr)
+                  const CodeGenSubRegIndex *SubReg = nullptr,
+                  bool IsDead = false)
       : OperandRenderer(OR_Register), InsnID(InsnID), TempRegID(TempRegID),
-        SubRegIdx(SubReg), IsDef(IsDef) {}
+        SubRegIdx(SubReg), IsDef(IsDef), IsDead(IsDead) {}
 
   static bool classof(const OperandRenderer *R) {
     return R->getKind() == OR_TempRegister;
   }
 
   void emitRenderOpcodes(MatchTable &Table, RuleMatcher &Rule) const override {
     if (SubRegIdx) {
       assert(!IsDef);
       Table << MatchTable::Opcode("GIR_AddTempSubRegister");
     } else
       Table << MatchTable::Opcode("GIR_AddTempRegister");
 
     Table << MatchTable::Comment("InsnID") << MatchTable::IntValue(InsnID)
           << MatchTable::Comment("TempRegID") << MatchTable::IntValue(TempRegID)
           << MatchTable::Comment("TempRegFlags");
 
-    if (IsDef)
-      Table << MatchTable::NamedValue("RegState::Define");
+    if (IsDef) {
+      if (IsDead)
+        Table << MatchTable::NamedValue("RegState::Define|RegState::Dead");
       else
+        Table << MatchTable::NamedValue("RegState::Define");
+    } else
       Table << MatchTable::IntValue(0);
 
     if (SubRegIdx)
       Table << MatchTable::NamedValue(SubRegIdx->getQualifiedName());
     Table << MatchTable::LineBreak;
   }
 };
 
 /// Adds a specific immediate to the instruction being built.
Expected<BuildMIAction &> createAndImportInstructionRenderer(
       RuleMatcher &M, InstructionMatcher &InsnMatcher,
       const TreePatternNode *Src, const TreePatternNode *Dst);
   Expected<action_iterator> createAndImportSubInstructionRenderer(
       action_iterator InsertPt, RuleMatcher &M, const TreePatternNode *Dst,
       unsigned TempReg);
   Expected<action_iterator>
   createInstructionRenderer(action_iterator InsertPt, RuleMatcher &M,
                             const TreePatternNode *Dst);
-  void importExplicitDefRenderers(BuildMIAction &DstMIBuilder);
+
+  Expected<action_iterator>
+  importExplicitDefRenderers(action_iterator InsertPt, RuleMatcher &M,
+                             BuildMIAction &DstMIBuilder,
+                             const TreePatternNode *Dst);
 
   Expected<action_iterator>
   importExplicitUseRenderers(action_iterator InsertPt, RuleMatcher &M,
                              BuildMIAction &DstMIBuilder,
                              const llvm::TreePatternNode *Dst);
   Expected<action_iterator>
   importExplicitUseRenderer(action_iterator InsertPt, RuleMatcher &Rule,
                             BuildMIAction &DstMIBuilder,
InsertPt = M.insertAction<BuildMIAction>(
         &Target.getInstruction(RK.getDef("COPY")));
     BuildMIAction &CopyToPhysRegMIBuilder =
         *static_cast<BuildMIAction *>(InsertPt->get());
     CopyToPhysRegMIBuilder.addRenderer<AddRegisterRenderer>(PhysInput.first,
                                                             true);
     CopyToPhysRegMIBuilder.addRenderer<CopyPhysRegRenderer>(PhysInput.first);
   }
 
-  importExplicitDefRenderers(DstMIBuilder);
+  if (auto Error = importExplicitDefRenderers(InsertPt, M, DstMIBuilder, Dst)
+                       .takeError())
+    return std::move(Error);
 
   if (auto Error = importExplicitUseRenderers(InsertPt, M, DstMIBuilder, Dst)
                        .takeError())
     return std::move(Error);
 
   return DstMIBuilder;
 }
 
Expected<action_iterator> GlobalISelEmitter::createInstructionRenderer(
   StringRef Name = DstI->TheDef->getName();
   if (Name == "COPY_TO_REGCLASS" || Name == "EXTRACT_SUBREG")
     DstI = &Target.getInstruction(RK.getDef("COPY"));
 
   return M.insertAction<BuildMIAction>(InsertPt, M.allocateOutputInsnID(),
                                        DstI);
 }
 
-void GlobalISelEmitter::importExplicitDefRenderers(
-    BuildMIAction &DstMIBuilder) {
+Expected<action_iterator> GlobalISelEmitter::importExplicitDefRenderers(
+    action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder,
+    const TreePatternNode *Dst) {
   const CodeGenInstruction *DstI = DstMIBuilder.getCGI();
+
   for (unsigned I = 0; I < DstI->Operands.NumDefs; ++I) {
     const CGIOperandList::OperandInfo &DstIOperand = DstI->Operands[I];
+    if (I > 0) {
+      // Patterns only handle a single result, so results after the first is an
+      // implicitly dead def.
+      const TypeSetByHwMode &ExtTy = Dst->getExtType(I);
+      if (!ExtTy.isMachineValueType())
+        return failedImport("unsupported typeset");
+
+      auto OpTy = MVTToLLT(ExtTy.getMachineValueType().SimpleTy);
+      if (!OpTy)
+        return failedImport("unsupported type");
+
+      unsigned TempRegID = M.allocateTempRegID();
+      InsertPt =
+          M.insertAction<MakeTempRegisterAction>(InsertPt, *OpTy, TempRegID);
+      DstMIBuilder.addRenderer<TempRegRenderer>(TempRegID, true, nullptr, true);
+      continue;
+    }
+
     DstMIBuilder.addRenderer<CopyRenderer>(DstIOperand.Name);
   }
+
+  return InsertPt;
 }
 
 Expected<action_iterator> GlobalISelEmitter::importExplicitUseRenderers(
     action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder,
     const llvm::TreePatternNode *Dst) {
   const CodeGenInstruction *DstI = DstMIBuilder.getCGI();
   CodeGenInstruction *OrigDstI = &Target.getInstruction(Dst->getOperator());
 
Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
   // Start with the defined operands (i.e., the results of the root operator).
   Record *DstOp = Dst->getOperator();
   if (!DstOp->isSubClassOf("Instruction"))
     return failedImport("Pattern operator isn't an instruction");
 
   auto &DstI = Target.getInstruction(DstOp);
   StringRef DstIName = DstI.TheDef->getName();
 
-  if (DstI.Operands.NumDefs != Src->getExtTypes().size())
+  if (DstI.Operands.NumDefs < Src->getExtTypes().size())
     return failedImport("Src pattern results and dst MI defs are different (" +
                         to_string(Src->getExtTypes().size()) + " def(s) vs " +
                         to_string(DstI.Operands.NumDefs) + " def(s))");
 
   // The root of the match also has constraints on the register bank so that it
   // matches the result instruction.
   unsigned OpIdx = 0;
   for (const TypeSetByHwMode &VTy : Src->getExtTypes()) {
     (void)VTy;
 
     const auto &DstIOperand = DstI.Operands[OpIdx];