Page MenuHomePhabricator

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

Authored by gargaroff on Jun 10 2020, 10:01 AM.

Details

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.

Diff Detail

Event Timeline

gargaroff created this revision.Jun 10 2020, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 10:01 AM
gargaroff added a comment.EditedJun 10 2020, 10:05 AM

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 added a subscriber: arsenm.Jun 10 2020, 10:37 AM

Can you add a test in test/MachineVerifier?

gargaroff updated this revision to Diff 270054.Jun 11 2020, 1:15 AM

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 added inline comments.Jun 11 2020, 9:45 AM
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 marked an inline comment as done.

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

gargaroff marked an inline comment as done.Jun 12 2020, 12:49 AM
gargaroff added inline comments.
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 retitled this revision from [GlobalISel] Add missing properties to G_BRINDIRECT, G_BRJT to [MachineVerifier][GlobalISel] Check that branches have a MBB operand or are declared indirect. Add missing properties to G_BRJT, G_BRINDIRECT.Jun 12 2020, 1:06 AM
gargaroff edited the summary of this revision. (Show Details)
gargaroff marked 2 inline comments as done.Jun 12 2020, 5:51 AM
gargaroff added inline comments.
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 accepted this revision.Jun 12 2020, 8:35 PM
dsanders added inline comments.
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

This revision is now accepted and ready to land.Jun 12 2020, 8:35 PM
arsenm added inline comments.Jun 13 2020, 6:22 AM
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

Move check to verifyPreISelGenericInstruction

This revision was automatically updated to reflect the committed changes.
arsenm added inline comments.Jun 15 2020, 6:34 AM
llvm/lib/CodeGen/MachineVerifier.cpp
876

Braces

llvm/test/MachineVerifier/test_g_brindirect_is_indirect_branch.mir
2

Don't need global-isel REQUIRES anymore

llvm/test/MachineVerifier/test_g_brjt_is_indirect_branch.mir
2

Don't need global-isel REQUIRES anymore

gargaroff marked 3 inline comments as done.Jun 15 2020, 7:37 AM