Page MenuHomePhabricator

[WIP] [ARM] Make MachineVerifier more strict about terminators
Changes PlannedPublic

Authored by efriedma on Nov 14 2017, 6:19 PM.

Details

Summary

Per a recent thread on llvmdev, MachineVerifier is a bit loose about the rule that terminators are always at the end of a basic block. Fix the ARM backend's analyzeBranch so it doesn't ignore predicated return instructions, and make the MachineVerifier rule more strict.

WIP because I'm not happy with the change in the generated code for atomic-cmpxchg.ll. IfConversion creates a BB which is a predicated return followed by an unconditional branch, and analyzeBranch can't analyze that, so we end up with a very stupid-looking branch after MachineBlockPlacement. Maybe we should be putting the unconditional branch into a separate block? Or should we be able to analyze the construct somehow? Or should we be avoiding IfConversion in cases where we can't fall through? (It might be possible to end up in a similar situation with a jump table branch followed by a unconditional branch, but I don't have a testcase for that.)

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Nov 14 2017, 6:19 PM
MatzeB edited edge metadata.Nov 15 2017, 10:58 AM

Uh, I wasn't aware that if-conversion could produce terminator instructions in the middle of a basic block, and I would suspect this to break other things like MachineBasicBlock::isReturnBlock() which assume terminators are at the end...

I must admit I don't understand how a change in analyzeBranch() allows you to drop that MachineVerifier special case. But I am all for dropping strange special cases in the machine verifier :)

IfConversion itself doesn't produce blocks with branches in the middle. It produces something like "bxne lr; b .LBB5_5" (a conditional return followed by an unconditional branch). From the verifier's perspective, that's fine because they're both terminators. But analyzeBranch was ignoring the conditional return, so MachineBlockPlacement would merge the block with the following block.

I would guess this didn't break anything because IfConversion runs pretty late in the pipeline.

uabelho edited edge metadata.Nov 17 2017, 1:21 AM

So now you make all returns (predicated or not) non-analyzable from analyzeBranch's perspective?

Do you know if all targets follow this? I suppose all tests under test/ still pass with the stricter
verifier checks, but have you looked at the different analyzeBranch implementations?

I think the change makes sense but I also know that fiddling with analyzeBranch can have
unexpected consequences so it's very hard to really know if it works or not.

So now you make all returns (predicated or not) non-analyzable from analyzeBranch's perspective?

Yes.

but have you looked at the different analyzeBranch implementations?

I just tried looking. I think this is consistent with what other targets do (e.g. PowerPC has a conditional return instruction, which it treats an unanalyzable).

I ran the stricter verifier tests on my out-of-tree target all weekend without finding anything
so that seems good at least.

efriedma planned changes to this revision.Aug 22 2018, 6:50 PM