[globalisel][tablegen] Support zero-instruction emission.

[dsanders]

Summary

Support the case where an operand of a pattern is also the whole of the
result pattern. In this case the original result and all its uses must be
replaced by the operand. However, register class restrictions can require
a COPY. This patch handles both cases by always emitting the copy and
leaving it for the register allocator to optimize.

Depends on D35833

[dsanders]

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

[dsanders]

In D36084#825975, @dsanders wrote:

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

It looks like it disappeared during a rebase and is already committed. That
being the case, I've just removed the C++ implementation of bitcast and
int_to_ptr that are covered by imported rules.

[dsanders]

Fix the previous update so that it contains both the original patch and the correction.

[rovka]

Hi Daniel,

Can you point me to the test that you've already committed? Was it an AArch64 inst select test? If so, I think you should also add a test in GlobalISelEmitter.td (I see one for a bitconvert -> COPY_TO_REGCLASS, but none checking mutations to COPY).

Thanks,
Diana

lib/Target/AArch64/AArch64InstructionSelector.cpp

1169 ↗

(On Diff #109074)

Can you point out those rules? The comments in SelectionDAGCompat say that G_INTTOPTR has no SelectionDAG equivalent, so I'm not sure where to look.

1171 ↗

(On Diff #109074)

Doesn't the destination always have to be a pointer and the source a scalar?

utils/TableGen/GlobalISelEmitter.cpp

2293 ↗

(On Diff #109074)

Would it make sense to move these 2 checks higher up, as a quick exit? (So we don't createAndImport the Src only to discover that Dst wasn't an instruction in the first place).

[dsanders]

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

In D36084#828962, @rovka wrote:

Hi Daniel,

Can you point me to the test that you've already committed? Was it an AArch64 inst select test? If so, I think you should also add a test in GlobalISelEmitter.td (I see one for a bitconvert -> COPY_TO_REGCLASS, but none checking mutations to COPY).

Thanks,
Diana

It was test/CodeGen/AArch64/GlobalISel/select-bitcast.mir and the imported rules that cover it are the bitconvert -> COPY_TO_REGCLASS rules which end up as COPY nodes with a constraint on the result. I considered treating COPY_TO_REGCLASS as a simple COPY but I decided against it in case it mattered on some targets.

lib/Target/AArch64/AArch64InstructionSelector.cpp

1169 ↗

(On Diff #109074)

I think I'm being confusing here. As you point out below, G_INTTOPTR always has a pointer result and as a result there are no rules that need to be imported to cover other cases.

1171 ↗

(On Diff #109074)

Good point. The only cases that we don't handle here are those that can't occur. I'll remove the if-statement.

1173 ↗

(On Diff #109074)

I've just noticed I'm missing a return false before this line.

1181 ↗

(On Diff #109074)

I've just noticed I'm missing a return false before this line.

utils/TableGen/GlobalISelEmitter.cpp

2293 ↗

(On Diff #109074)

We could do that. The only reason it comes after the matcher is to keep a boundary between the code that handles the matcher and the code that handles the renderer. The two were tangled together in the early versions of GlobalISelEmitter.cpp.

[dsanders]

Fix the issues with the changes to the C++ implementation of G_INTTOPTR and G_BITCAST.

[dsanders]

utils/TableGen/GlobalISelEmitter.cpp

2293 ↗

(On Diff #109074)

I just went to do this and I don't think it can be moved up without significant changes. The Dst->getOperator() on line 2285 requires that Dst->isLeaf() is false so it must either come after the if-statement on line 2255 or be wrapped in a if (!Dst->isLeaf()) {}. If we choose to move the if-statement on line 2255 up as well then we're creating the renderer before the matcher which will break getOperand(StringRef) because the variables (defineInsnVar()) wont be defined at that point. If we chose to wrap it in an if (!Dst->isLeaf()) and move it past line 2255's if-statement then we have a use-beyond-scope issue to fix and fixing that requires allowing DstOp to be null and rewriting DstI to be a (possibly-null) pointer which will impact the createAndImportInstructionRenderer() function and those it calls.

[rovka]

In D36084#830458, @dsanders wrote:

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

LGTM if you add that test.

utils/TableGen/GlobalISelEmitter.cpp

2295 ↗

(On Diff #110196)

Typo: its

2293 ↗

(On Diff #109074)

Ok, it's fine as it is then.

[dsanders]

Two windows bots failed select-inc.mir which should have been unaffected by the change. Investigating

llvm/trunk/lib/Target/AArch64/AArch64InstructionSelector.cpp

case TargetOpcode::G_FPTOUI: {
     I.setDesc(TII.get(NewOpc));
     constrainSelectedInstRegOperands(I, TII, TRI, RBI);
 
     return true;
   }
 
 
   case TargetOpcode::G_INTTOPTR:
+    // The importer is currently unable to import pointer types since they
+    // didn't exist in SelectionDAG.
+    return selectCopy(I, TII, MRI, TRI, RBI);
+
   case TargetOpcode::G_BITCAST:
+    // Imported SelectionDAG rules can handle every bitcast except those that
+    // bitcast from a type to the same type. Ideally, these shouldn't occur
+    // but we might not run an optimizer that deletes them.
+    if (MRI.getType(I.getOperand(0).getReg()) ==
+        MRI.getType(I.getOperand(1).getReg()))
       return selectCopy(I, TII, MRI, TRI, RBI);
+    return false;
 
   case TargetOpcode::G_FPEXT: {
     if (MRI.getType(I.getOperand(0).getReg()) != LLT::scalar(64)) {
       DEBUG(dbgs() << "G_FPEXT to type " << Ty
                    << ", expected: " << LLT::scalar(64) << '\n');
       return false;
     }
 

llvm/trunk/test/CodeGen/AArch64/GlobalISel/select-bitcast.mir

 # RUN: llc -O0 -mtriple=aarch64-- -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s
 
 --- |
   target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
 
   define void @bitcast_s32_gpr() { ret void }
   define void @bitcast_s32_fpr() { ret void }
   define void @bitcast_s32_gpr_fpr() { ret void }
   define void @bitcast_s32_fpr_gpr() { ret void }
   define void @bitcast_s64_gpr() { ret void }
   define void @bitcast_s64_fpr() { ret void }
   define void @bitcast_s64_gpr_fpr() { ret void }
   define void @bitcast_s64_fpr_gpr() { ret void }
+  define void @bitcast_s64_v2f32_fpr() { ret void }
+  define void @bitcast_s64_v8i8_fpr() { ret void }
 ...
 
 ---
 # CHECK-LABEL: name: bitcast_s32_gpr
 name:            bitcast_s32_gpr
 legalized:       true
 regBankSelected: true
 # CHECK:      registers:

 body:             |
   bb.0:
     liveins: %d0
 
     %0(s64) = COPY %d0
     %1(s64) = G_BITCAST %0
     %x0 = COPY %1(s64)
 ...
+
+---
+# CHECK-LABEL: name: bitcast_s64_v2f32_fpr
+name:            bitcast_s64_v2f32_fpr
+legalized:       true
+regBankSelected: true
+
+# CHECK:      registers:
+# CHECK-NEXT:  - { id: 0, class: fpr64, preferred-register: '' }
+# CHECK-NEXT:  - { id: 1, class: fpr64, preferred-register: '' }
+registers:
+  - { id: 0, class: fpr }
+  - { id: 1, class: fpr }
+
+# CHECK:  body:
+# CHECK:    %0 = COPY %d0
+# CHECK:    %1 = COPY %0
+body:             |
+  bb.0:
+    liveins: %d0
+
+    %0(s64) = COPY %d0
+    %1(<2 x s32>) = G_BITCAST %0
+    %x0 = COPY %1(<2 x s32>)
+...
+
+---
+# CHECK-LABEL: name: bitcast_s64_v8i8_fpr
+name:            bitcast_s64_v8i8_fpr
+legalized:       true
+regBankSelected: true
+
+# CHECK:      registers:
+# CHECK-NEXT:  - { id: 0, class: fpr64, preferred-register: '' }
+# CHECK-NEXT:  - { id: 1, class: fpr64, preferred-register: '' }
+registers:
+  - { id: 0, class: fpr }
+  - { id: 1, class: fpr }
+
+# CHECK:  body:
+# CHECK:    %0 = COPY %d0
+# CHECK:    %1 = COPY %0
+body:             |
+  bb.0:
+    liveins: %d0
+
+    %0(s64) = COPY %d0
+    %1(<8 x s8>) = G_BITCAST %0
+    %x0 = COPY %1(<8 x s8>)
+...

llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp

 /// Helper function to let the emitter report skip reason error messages.
 static Error failedImport(const Twine &Reason) {
   return make_error<StringError>(Reason, inconvertibleErrorCode());
 }
 
 static Error isTrivialOperatorNode(const TreePatternNode *N) {
   std::string Explanation = "";
   std::string Separator = "";
-  if (N->isLeaf()) {
-    if (isa<IntInit>(N->getLeafValue()))
-      return Error::success();
-
-    Explanation = "Is a leaf";
-    Separator = ", ";
-  }
-
   if (N->hasAnyPredicate()) {
     Explanation = Separator + "Has a predicate (" + explainPredicates(N) + ")";
     Separator = ", ";
   }
 
   if (N->getTransformFn()) {
     Explanation += Separator + "Has a transform function";
     Separator = ", ";
   }
 
-  if (!N->isLeaf() && !N->hasAnyPredicate() && !N->getTransformFn())
+  if (!N->hasAnyPredicate() && !N->getTransformFn())
     return Error::success();
 
   return failedImport(Explanation);
 }
 
 static Record *getInitValueAsRegClass(Init *V) {
   if (DefInit *VDefInit = dyn_cast<DefInit>(V)) {
     if (VDefInit->getDef()->isSubClassOf("RegisterOperand"))

 }
 
 bool OperandPredicateMatcher::isHigherPriorityThan(
     const OperandPredicateMatcher &B) const {
   // Generally speaking, an instruction is more important than an Int or a
   // LiteralInt because it can cover more nodes but theres an exception to
   // this. G_CONSTANT's are less important than either of those two because they
   // are more permissive.
-  if (const InstructionOperandMatcher *AOM =
-          dyn_cast<InstructionOperandMatcher>(this)) {
-    if (AOM->getInsnMatcher().isConstantInstruction()) {
-      if (B.Kind == OPM_Int) {
-        return false;
-      }
-    }
-  }
-  if (const InstructionOperandMatcher *BOM =
-          dyn_cast<InstructionOperandMatcher>(&B)) {
-    if (BOM->getInsnMatcher().isConstantInstruction()) {
-      if (Kind == OPM_Int) {
-        return true;
-      }
-    }
-  }
+
+  const InstructionOperandMatcher *AOM =
+      dyn_cast<InstructionOperandMatcher>(this);
+  const InstructionOperandMatcher *BOM =
+      dyn_cast<InstructionOperandMatcher>(&B);
+  bool AIsConstantInsn = AOM && AOM->getInsnMatcher().isConstantInstruction();
+  bool BIsConstantInsn = BOM && BOM->getInsnMatcher().isConstantInstruction();
+
+  if (AOM && BOM) {
+    // The relative priorities between a G_CONSTANT and any other instruction
+    // don't actually matter but this code is needed to ensure a strict weak
+    // ordering. This is particularly important on Windows where the rules will
+    // be incorrectly sorted without it.
+    if (AIsConstantInsn != BIsConstantInsn)
+      return AIsConstantInsn < BIsConstantInsn;
+
+  if (AOM && AIsConstantInsn && (B.Kind == OPM_Int || B.Kind == OPM_LiteralInt))
+    return false;
+  if (BOM && BIsConstantInsn && (Kind == OPM_Int || Kind == OPM_LiteralInt))
 
   return Kind < B.Kind;
 }
 
 //===- GlobalISelEmitter class --------------------------------------------===//
 
 class GlobalISelEmitter {
 public:
Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
   // If the root of either pattern isn't a simple operator, ignore it.
   if (auto Err = isTrivialOperatorNode(Dst))
     return failedImport("Dst pattern root isn't a trivial operator (" +
                         toString(std::move(Err)) + ")");
   if (auto Err = isTrivialOperatorNode(Src))
     return failedImport("Src pattern root isn't a trivial operator (" +
                         toString(std::move(Err)) + ")");
 
-  if (Dst->isLeaf())
+  InstructionMatcher &InsnMatcherTemp = M.addInstructionMatcher(Src->getName());
+  unsigned TempOpIdx = 0;
+  auto InsnMatcherOrError =
+      createAndImportSelDAGMatcher(InsnMatcherTemp, Src, TempOpIdx);
+  if (auto Error = InsnMatcherOrError.takeError())
+    return std::move(Error);
+  InstructionMatcher &InsnMatcher = InsnMatcherOrError.get();
+
+  if (Dst->isLeaf()) {
+    Record *RCDef = getInitValueAsRegClass(Dst->getLeafValue());
+
+    const CodeGenRegisterClass &RC = Target.getRegisterClass(RCDef);
+    if (RCDef) {
+      // We need to replace the def and all its uses with the specified
+      // operand. However, we must also insert COPY's wherever needed.
+      // For now, emit a copy and let the register allocator clean up.
+      auto &DstI = Target.getInstruction(RK.getDef("COPY"));
+      const auto &DstIOperand = DstI.Operands[0];
+
+      OperandMatcher &OM0 = InsnMatcher.getOperand(0);
+      OM0.setSymbolicName(DstIOperand.Name);
+      OM0.addPredicate<RegisterBankOperandMatcher>(RC);
+
+      auto &DstMIBuilder = M.addAction<BuildMIAction>(0, &DstI, InsnMatcher);
+      DstMIBuilder.addRenderer<CopyRenderer>(0, InsnMatcher, DstIOperand.Name);
+      DstMIBuilder.addRenderer<CopyRenderer>(0, InsnMatcher, Dst->getName());
+      M.addAction<ConstrainOperandToRegClassAction>(0, 0, RC);
+
+      // We're done with this pattern!  It's eligible for GISel emission; return
+      // it.
+      ++NumPatternImported;
+      return std::move(M);
+    }
+
     return failedImport("Dst pattern root isn't a known leaf");
+  }
 
   // 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);
   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))");
 
-  InstructionMatcher &InsnMatcherTemp = M.addInstructionMatcher(Src->getName());
-  unsigned TempOpIdx = 0;
-  auto InsnMatcherOrError =
-      createAndImportSelDAGMatcher(InsnMatcherTemp, Src, TempOpIdx);
-  if (auto Error = InsnMatcherOrError.takeError())
-    return std::move(Error);
-  InstructionMatcher &InsnMatcher = InsnMatcherOrError.get();
-
   // The root of the match also has constraints on the register bank so that it
   // matches the result instruction.
   unsigned OpIdx = 0;
   for (const EEVT::TypeSet &Ty : Src->getExtTypes()) {
     (void)Ty;
 
     const auto &DstIOperand = DstI.Operands[OpIdx];
     Record *DstIOpRec = DstIOperand.Rec;
SubtargetFeatureInfo::emitComputeAvailableFeatures(
       Target.getName(), "InstructionSelector",
       "computeAvailableFunctionFeatures", FunctionFeatures, OS,
       "const MachineFunction *MF");
 
   // Emit a table containing the LLT objects needed by the matcher and an enum
   // for the matcher to reference them with.
   std::vector<LLTCodeGen> TypeObjects = {
       LLT::scalar(8),      LLT::scalar(16),     LLT::scalar(32),
-      LLT::scalar(64),     LLT::scalar(80),     LLT::vector(8, 1),
-      LLT::vector(16, 1),  LLT::vector(32, 1),  LLT::vector(64, 1),
-      LLT::vector(8, 8),   LLT::vector(16, 8),  LLT::vector(32, 8),
-      LLT::vector(64, 8),  LLT::vector(4, 16),  LLT::vector(8, 16),
-      LLT::vector(16, 16), LLT::vector(32, 16), LLT::vector(2, 32),
-      LLT::vector(4, 32),  LLT::vector(8, 32),  LLT::vector(16, 32),
-      LLT::vector(2, 64),  LLT::vector(4, 64),  LLT::vector(8, 64),
+      LLT::scalar(64),     LLT::scalar(80),     LLT::scalar(128),
+      LLT::vector(8, 1),   LLT::vector(16, 1),  LLT::vector(32, 1),
+      LLT::vector(64, 1),  LLT::vector(8, 8),   LLT::vector(16, 8),
+      LLT::vector(32, 8),  LLT::vector(64, 8),  LLT::vector(4, 16),
+      LLT::vector(8, 16),  LLT::vector(16, 16), LLT::vector(32, 16),
+      LLT::vector(2, 32),  LLT::vector(4, 32),  LLT::vector(8, 32),
+      LLT::vector(16, 32), LLT::vector(2, 64),  LLT::vector(4, 64),
+      LLT::vector(8, 64),
   };
   std::sort(TypeObjects.begin(), TypeObjects.end());
   OS << "enum {\n";
   for (const auto &TypeObject : TypeObjects) {
     OS << "  ";
     TypeObject.emitCxxEnumValue(OS);
     OS << ",\n";
   }