[MachineVerifier][GlobalISel] Checking that generic instrs have LLTs on all vregs

[rtereshin]

Summary

Every generic machine instruction must have generic virtual registers
only, that is, have a low-level type attached to each operand.

Previously MachineVerifier would catch a type missing on an operand
only if the previous operand for the the same type index exists and
have a type attached to it and it will report it as a type mismatch.
This is incosistent behaviour and a misleading error message.

This commit makes sure MachineVerifier explicitly checks that the
types are there for every operand and if not provides a
straightforward error message.

[rtereshin]

The patch relies on an NFC not included in the diff:

[MachineVerifier][GlobalISel] NFC, Improving MO printing and refactoring visitMachineInstrBefore

This is an NFC pre-commit for the following "Checking that generic
instrs have LLTs on all vregs" commit.

This overloads MachineOperand::print to make it possible to print LLTs
with standalone machine operands.

This also overloads MachineVerifier::print(...MachineOperand...) with
an optional LLT using the newly introduced MachineOperand::print
variant; no actual calls added.

This also refactors MachineVerifier::visitMachineInstrBefore in the
parts dealing with all generic instructions (checking Selected
property, LLTs, and phys regs).

[rtereshin]

Splitting up tests between the NFC and the main commit in a better way.

[rtereshin]

Cleaning up NFC a bit.

[qcolombet]

Nice!

[rtereshin]

Thank you, @qcolombet!

lib/CodeGen/MachineInstr.cpp

LLT MachineInstr::getTypeToPrint(unsigned OpIdx, SmallBitVector &PrintedTypes,
 
   auto &OpInfo = getDesc().OpInfo[OpIdx];
   if (!OpInfo.isGenericType())
     return MRI.getType(Op.getReg());
 
   if (PrintedTypes[OpInfo.getGenericTypeIndex()])
     return LLT{};
 
+  LLT TypeToPrint = MRI.getType(Op.getReg());
+  // Don't mark the type index printed if it wasn't actually printed: maybe
+  // another operand with the same type index has an actual type attached:
+  if (TypeToPrint.isValid())
     PrintedTypes.set(OpInfo.getGenericTypeIndex());
-  return MRI.getType(Op.getReg());
+  return TypeToPrint;
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_DUMP_METHOD void MachineInstr::dump() const {
   dbgs() << "  ";
   print(dbgs());
 }
 #endif

lib/CodeGen/MachineVerifier.cpp

for (unsigned I = 0; I < MCID.getNumOperands(); ++I) {
         continue;
       // Generic instructions specify type equality constraints between some of
       // their operands. Make sure these are consistent.
       size_t TypeIdx = MCID.OpInfo[I].getGenericTypeIndex();
       Types.resize(std::max(TypeIdx + 1, Types.size()));
 
       const MachineOperand *MO = &MI->getOperand(I);
       LLT OpTy = MRI->getType(MO->getReg());
-      if (Types[TypeIdx].isValid() && Types[TypeIdx] != OpTy)
-        report("type mismatch in generic instruction", MI);
+      // Don't report a type mismatch if there is no actual mismatch, only a
+      // type missing, to reduce noise:
+      if (OpTy.isValid()) {
+        // Only the first valid type for a type index will be printed: don't
+        // overwrite it later so it's always clear which type was expected:
+        if (!Types[TypeIdx].isValid())
           Types[TypeIdx] = OpTy;
+        else if (Types[TypeIdx] != OpTy)
+          report("Type mismatch in generic instruction", MO, I, OpTy);
+      } else {
+        // Generic instructions must have types attached to their operands.
+        report("Generic instruction is missing a virtual register type", MO, I);
+      }
     }
 
     // Generic opcodes must not have physical register operands.
     for (unsigned I = 0; I < MI->getNumOperands(); ++I) {
       const MachineOperand *MO = &MI->getOperand(I);
       if (MO->isReg() && TargetRegisterInfo::isPhysicalRegister(MO->getReg()))
-        report("Generic instruction cannot have physical register", MI);
+        report("Generic instruction cannot have physical register", MO, I);
     }
   }
 
   StringRef ErrorInfo;
   if (!TII->verifyInstruction(*MI, ErrorInfo))
     report(ErrorInfo.data(), MI);
 
   // Verify properties of various specific instruction types

test/CodeGen/X86/verifier-generic-types-1.mir

-# RUN: not llc -o - %s -mtriple=x86_64-- -verify-machineinstrs -run-pass=none 2>&1
-# XFAIL: *
+# RUN: not llc -o - %s -mtriple=x86_64-- -verify-machineinstrs -run-pass=none 2>&1 | FileCheck %s
+
+# CHECK-NOT: Type mismatch
+
+# CHECK: Bad machine code: Generic instruction is missing a virtual register type
+# CHECK-NEXT: - function:    first_type_of_a_type_index_missing_and_no_mismatches
+# CHECK-NEXT: - basic block: %bb.0
+# CHECK-NEXT: - instruction: %2:gr64 = G_ADD %0:_(s64), %1:gr64
+# CHECK-NEXT: - operand 0:   %2:gr64
+
+# CHECK-NOT: Type mismatch
+
+# CHECK: Bad machine code: Generic instruction is missing a virtual register type
+# CHECK-NEXT: - function:    first_type_of_a_type_index_missing_and_no_mismatches
+# CHECK-NEXT: - basic block: %bb.0
+# CHECK-NEXT: - instruction: %2:gr64 = G_ADD %0:_(s64), %1:gr64
+# CHECK-NEXT: - operand 2:   %1:gr64
+
+# CHECK-NOT: Type mismatch
 
 ---
 name:              first_type_of_a_type_index_missing_and_no_mismatches
 tracksRegLiveness: true
 body:              |
   bb.0:
     liveins: $rdi, $rsi
 
     %0:_(s64) = COPY $rdi
     %1:gr64 = COPY $rsi
     %2:gr64 = G_ADD %0, %1
     $rax = COPY %2
 ...

test/CodeGen/X86/verifier-generic-types-2.mir

 # RUN: not llc -o - %s -mtriple=x86_64-- -verify-machineinstrs -run-pass=none 2>&1 | FileCheck %s
 
-# CHECK: Bad machine code: type mismatch in generic instruction
+# CHECK: Bad machine code: Generic instruction is missing a virtual register type
 # CHECK-NEXT: - function:    first_type_of_a_type_index_missing_and_a_mismatch
 # CHECK-NEXT: - basic block: %bb.0
-# CHECK-NEXT: - instruction: %2:gr64 = G_ADD %0:_, %1:_
+# CHECK-NEXT: - instruction: %2:gr64 = G_ADD %0:_(s64), %1:_
+# CHECK-NEXT: - operand 0:   %2:gr64
+
+# CHECK: Bad machine code: Type mismatch in generic instruction
+# CHECK-NEXT: - function:    first_type_of_a_type_index_missing_and_a_mismatch
+# CHECK-NEXT: - basic block: %bb.0
+# CHECK-NEXT: - instruction: %2:gr64 = G_ADD %0:_(s64), %1:_
+# CHECK-NEXT: - operand 2:   %1:_(s32)
 
 ---
 name:              first_type_of_a_type_index_missing_and_a_mismatch
 tracksRegLiveness: true
 body:              |
   bb.0:
     liveins: $rdi, $esi
 
     %0:_(s64) = COPY $rdi
     %1:_(s32)= COPY $esi
     %2:gr64 = G_ADD %0, %1
     $rax = COPY %2
 ...