[PATCH] D36084: [globalisel][tablegen] Support zero-instruction emission.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

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

In https://reviews.llvm.org/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.



================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1169
+    // 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() ||
----------------
rovka wrote:
> 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. 
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.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1171
+    if (MRI.getType(I.getOperand(0).getReg()).isPointer() ||
+        MRI.getType(I.getOperand(1).getReg()).isPointer())
+      return selectCopy(I, TII, MRI, TRI, RBI);
----------------
rovka wrote:
> Doesn't the destination always have to be a pointer and the source a scalar?
Good point. The only cases that we don't handle here are those that can't occur. I'll remove the if-statement.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1173
+      return selectCopy(I, TII, MRI, TRI, RBI);
   case TargetOpcode::G_BITCAST:
+    // Imported SelectionDAG rules can handle every bitcast except those that
----------------
I've just noticed I'm missing a `return false` before this line.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1181
 
   case TargetOpcode::G_FPEXT: {
     if (MRI.getType(I.getOperand(0).getReg()) != LLT::scalar(64)) {
----------------
I've just noticed I'm missing a `return false` before this line.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2293
                         to_string(Src->getExtTypes().size()) + " def(s) vs " +
                         to_string(DstI.Operands.NumDefs) + " def(s))");
 
----------------
rovka wrote:
> 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).
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.


https://reviews.llvm.org/D36084