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

Fri Jun 12 05:53:53 PDT 2020

gargaroff marked 2 inline comments as done.
gargaroff added inline comments.

================
Comment at: llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir:17
+
+    ; CHECK-LABEL: test_indirect_branch
+    G_BRINDIRECT %0
----------------
gargaroff wrote:
> dsanders wrote:
> > 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
> 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.
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81587/new/

https://reviews.llvm.org/D81587