[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

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

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

utils/TableGen/GlobalISelEmitter.cpp

2293

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

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

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

1173

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

1181

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

utils/TableGen/GlobalISelEmitter.cpp

2293

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

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

2260

Typo: its

2293

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

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:
+    // Imported SelectionDAG rules can handle everything except pointer types
+    // which didn't exist in that instruction selector.
+    if (MRI.getType(I.getOperand(0).getReg()).isPointer() ||
+        MRI.getType(I.getOperand(1).getReg()).isPointer())
+      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);
 
   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;
     }
 
     if (MRI.getType(I.getOperand(1).getReg()) != LLT::scalar(32)) {
       DEBUG(dbgs() << "G_FPEXT from type " << Ty

utils/TableGen/GlobalISelEmitter.cpp

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 it's 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))");
 
   // 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];