[MachineVerifier][GlobalISel] Check that branches have a MBB operand or are declared indirect. Add missing properties to G_BRJT, G_BRINDIRECT

[gargaroff]

Summary

Teach MachineVerifier to check branches for MBB operands if they are not declared indirect.

Add isBarrier, isIndirectBranch to G_BRINDIRECT and G_BRJT.
Without these, MachineInstr.isConditionalBranch() was giving a
false-positive for those instructions.

[gargaroff]

I noticed the missing properties when implementing analyzeBranch for our downstream target. Since the machine verifier also uses this function, all of the GMIR was passing through it and we were getting false-positives for those two instructions. Seems like an obvious fix. I guess the two tests changed because now there is no implicit fall-through, which marks the basic block as a successor.

[arsenm]

Can you add a test in test/MachineVerifier?

[gargaroff]

Add tests to test/MachineVerifier

The tests use RISCV's implementation of analyzeBranch, which relies on MCInstrDesc. Without the fix applied to G_BRJT, G_BRINDIRECT, those two tests crash.

[dsanders]

llvm/test/CodeGen/AArch64/GlobalISel/legalize-blockaddress.mir

28

I feel that this should probably become successors: deref(%123)(0x80000000) or something along those lines but that's not an issue for this patch. It's more of a potential improvement to MIR itself

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir

18

Most of the verifier tests are checking for specific messages output by the verifier. Is it possible to do that for these two tests too? For this one, I'm thinking maybe it could test for an isBranch() and !isIndirectBranch() that doesn't have any operands that directly reference basic blocks. It would then report that isIndirectBranch() is missing for instructions like that. That would also cover target-specific indirect branch instructions that forget isIndirectBranch() too

[gargaroff]

Teach machine verifier to check branch instructions for basic block
operands, if they are not declared indirect.

[gargaroff]

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir

18

I'm not sure it would be possible for this test. We could implement that behavior in the verifier and then write such a test, however since I'm actually fixing both of those instructions with this patch, none of them would create the actual message that we're looking for. We could only do a CHECK-NOT I guess.

Apart from that, I do think that the check you propose is something desirable to have.

[gargaroff]

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir

18

Well, this didn't work too well. A lot of target specific instructions (especially pseudos) are causing troubles and while some of them are easy to fix, many aren't as straight forward.

I would therefore suggest to limit this check to either generic opcodes or to exclude pseudo instructions from this check. Thoughts?

[dsanders]

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir

18

I'm actually fixing both of those instructions with this patch, none of them would create the actual message that we're looking for

Good point :-)

We could only do a CHECK-NOT I guess.

Hmm, tests that only do a CHECK-NOT have a tendency to pass for bad reasons like spelling changes and so on. It's somewhat useful for documentation but functionally it will be about the same as a don't-crash test. Given that we can't test for the emission of the message, let's go with the CHECK-NOT if only to document the message we would get if it wasn't already fixed

I would therefore suggest to limit this check to either generic opcodes or to exclude pseudo instructions from this check. Thoughts?

I think that makes sense. There's also target specific generic opcodes* which would be covered by this so it would be ensuring those are configured correctly too.

*When we eventually add proper support for that. They provide a similar feature to custom SDNode's. We use them in our target but there's nothing actually target specific about them yet as we just pollute the common numberspace

[arsenm]

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir

18

What’s wrong with the current support? It’s just a bit in the TSFlags

[gargaroff]

Move check to verifyPreISelGenericInstruction

[arsenm]

llvm/lib/CodeGen/MachineVerifier.cpp

876

Braces

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir

3

Don't need global-isel REQUIRES anymore

llvm/test/MachineVerifier/test_g_brjt_is_indirect_branch.mir

3

Don't need global-isel REQUIRES anymore

llvm/include/llvm/Target/GenericOpcodes.td

 
 // Generic indirect branch.
 def G_BRINDIRECT : GenericInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins type0:$src1);
   let hasSideEffects = 0;
   let isBranch = 1;
   let isTerminator = 1;
+  let isBarrier = 1;
+  let isIndirectBranch = 1;
 }
 
 // Generic branch to jump table entry
 def G_BRJT : GenericInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins ptype0:$tbl, unknown:$jti, type1:$idx);
   let hasSideEffects = 0;
   let isBranch = 1;
   let isTerminator = 1;
+  let isBarrier = 1;
+  let isIndirectBranch = 1;
 }
 
 def G_READ_REGISTER : GenericInstruction {
   let OutOperandList = (outs type0:$dst);
   let InOperandList = (ins unknown:$register);
   let hasSideEffects = 1;
 
   // Assume convergent. It's probably not worth the effort of somehow

llvm/lib/CodeGen/MachineVerifier.cpp

void MachineVerifier::verifyPreISelGenericInstruction(const MachineInstr *MI) {
   for (unsigned I = 0, E = std::min(MCID.getNumOperands(), NumOps);
        I != E; ++I) {
     if (!MCID.OpInfo[I].isGenericType())
       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);
     if (!MO->isReg()) {
       report("generic instruction must use register operands", MI);
       continue;
     }
 
     LLT OpTy = MRI->getType(MO->getReg());
     // Don't report a type mismatch if there is no actual mismatch, only a
if (MI->isDebugInstr()) {
       if (mapped)
         report("Instruction inside bundle has a slot index", MI);
     } else {
       if (!mapped)
         report("Missing slot index", MI);
     }
   }
 
+  // Branches must reference a basic block if they are not indirect
+  if (MI->isBranch() && !MI->isIndirectBranch()) {
+    bool HasMBB = false;
+    for (const MachineOperand &Op : MI->operands()) {
+      if (Op.isMBB()) {
+        HasMBB = true;
+        break;
+      }
+    }
+
+    if (!HasMBB)
+      report("Branch instruction is missing a basic block operand or "
+             "isIndirectBranch property",
+             MI);
+  }
+
   if (isPreISelGenericOpcode(MCID.getOpcode())) {
     verifyPreISelGenericInstruction(MI);
     return;
   }
 
   StringRef ErrorInfo;
   if (!TII->verifyInstruction(*MI, ErrorInfo))
     report(ErrorInfo.data(), MI);

llvm/test/CodeGen/AArch64/GlobalISel/legalize-blockaddress.mir

 alignment:       4
 tracksRegLiveness: true
 registers:
   - { id: 0, class: _ }
   - { id: 1, class: _ }
 body:             |
   ; CHECK-LABEL: name: test_blockaddress
   ; CHECK: bb.0 (%ir-block.0):
-  ; CHECK:   successors: %bb.1(0x80000000)
   ; CHECK:   [[BLOCK_ADDR:%[0-9]+]]:_(p0) = G_BLOCK_ADDR blockaddress(@test_blockaddress, %ir-block.block)
   ; CHECK:   [[ADRP:%[0-9]+]]:gpr64(p0) = ADRP target-flags(aarch64-page) @addr
   ; CHECK:   [[ADD_LOW:%[0-9]+]]:_(p0) = G_ADD_LOW [[ADRP]](p0), target-flags(aarch64-pageoff, aarch64-nc) @addr
   ; CHECK:   G_STORE [[BLOCK_ADDR]](p0), [[ADD_LOW]](p0) :: (store 8 into @addr)
   ; CHECK:   G_BRINDIRECT [[BLOCK_ADDR]](p0)
   ; CHECK: bb.1.block (address-taken):
   ; CHECK:   RET_ReallyLR
   bb.1 (%ir-block.0):

llvm/test/CodeGen/AArch64/GlobalISel/select-blockaddress.mir

 regBankSelected: true
 tracksRegLiveness: true
 registers:
   - { id: 0, class: gpr }
   - { id: 1, class: gpr }
 body:             |
   ; CHECK-LABEL: name: test_blockaddress
   ; CHECK: bb.0 (%ir-block.0):
-  ; CHECK:   successors: %bb.1(0x80000000)
   ; CHECK:   [[MOVaddrBA:%[0-9]+]]:gpr64 = MOVaddrBA target-flags(aarch64-page) blockaddress(@test_blockaddress, %ir-block.block), target-flags(aarch64-pageoff, aarch64-nc) blockaddress(@test_blockaddress, %ir-block.block)
   ; CHECK:   [[MOVaddr:%[0-9]+]]:gpr64common = MOVaddr target-flags(aarch64-page) @addr, target-flags(aarch64-pageoff, aarch64-nc) @addr
   ; CHECK:   STRXui [[MOVaddrBA]], [[MOVaddr]], 0 :: (store 8 into @addr)
   ; CHECK:   BR [[MOVaddrBA]]
   ; CHECK: bb.1.block (address-taken):
   ; CHECK:   RET_ReallyLR
   ; LARGE-LABEL: name: test_blockaddress
   ; LARGE: bb.0 (%ir-block.0):
-  ; LARGE:   successors: %bb.1(0x80000000)
   ; LARGE:   [[MOVZXi:%[0-9]+]]:gpr64 = MOVZXi target-flags(aarch64-g0, aarch64-nc) blockaddress(@test_blockaddress, %ir-block.block), 0
   ; LARGE:   [[MOVKXi:%[0-9]+]]:gpr64 = MOVKXi [[MOVZXi]], target-flags(aarch64-g1, aarch64-nc) blockaddress(@test_blockaddress, %ir-block.block), 16
   ; LARGE:   [[MOVKXi1:%[0-9]+]]:gpr64 = MOVKXi [[MOVKXi]], target-flags(aarch64-g2, aarch64-nc) blockaddress(@test_blockaddress, %ir-block.block), 32
   ; LARGE:   [[MOVKXi2:%[0-9]+]]:gpr64 = MOVKXi [[MOVKXi1]], target-flags(aarch64-g3) blockaddress(@test_blockaddress, %ir-block.block), 48
   ; LARGE:   [[MOVZXi1:%[0-9]+]]:gpr64 = MOVZXi target-flags(aarch64-g0, aarch64-nc) @addr, 0
   ; LARGE:   [[MOVKXi3:%[0-9]+]]:gpr64 = MOVKXi [[MOVZXi1]], target-flags(aarch64-g1, aarch64-nc) @addr, 16
   ; LARGE:   [[MOVKXi4:%[0-9]+]]:gpr64 = MOVKXi [[MOVKXi3]], target-flags(aarch64-g2, aarch64-nc) @addr, 32
   ; LARGE:   [[MOVKXi5:%[0-9]+]]:gpr64common = MOVKXi [[MOVKXi4]], target-flags(aarch64-g3) @addr, 48

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir

+# RUN: llc -march=riscv32 -o - -run-pass=none -verify-machineinstrs %s | FileCheck %s
+# REQUIRES: global-isel, riscv-registered-target
+
+# This test checks that the G_BRINDIRECT is an indirect branch by leveraging
+# RISCV's version of analyzeBranch. If G_BRINDIRECT would not be an indirect
+# branch, this test would crash.
+
+---
+name:            test_indirect_branch
+legalized:       true
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $x0
+    %0:_(p0) = COPY $x0
+
+    ; CHECK-NOT: Branch instruction is missing a basic block operand or isIndirectBranch property
+    G_BRINDIRECT %0
+
+...

llvm/test/MachineVerifier/test_g_brjt_is_indirect_branch.mir

+# RUN: llc -march=riscv32 -o - -run-pass=none -verify-machineinstrs %s | FileCheck %s
+# REQUIRES: global-isel, riscv-registered-target
+
+# This test checks that the G_BRJT is an indirect branch by leveraging RISCV's
+# version of analyzeBranch. If G_BRJT would not be an indirect branch, this
+# test would crash.
+
+---
+name:            test_jump_table
+legalized:       true
+tracksRegLiveness: true
+jumpTable:
+  kind:            block-address
+  entries:
+    - id:              0
+      blocks:          [ '%bb.0' ]
+body:             |
+  bb.0:
+    liveins: $x0
+    %0:_(s32) = COPY $x0
+    %1:_(p0) = COPY $x0
+
+    ; CHECK-NOT: Branch instruction is missing a basic block operand or isIndirectBranch property
+    G_BRJT %1, %jump-table.0, %0
+
+...