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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir:17
+
+    ; CHECK-LABEL: test_indirect_branch
+    G_BRINDIRECT %0
----------------
dsanders wrote:
> gargaroff wrote:
> > 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?
> >  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
What’s wrong with the current support? It’s just a bit in the TSFlags


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81587