This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Make MachineVerifier more strict about terminators
ClosedPublic

Authored by samparker 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

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
samparker commandeered this revision.Aug 19 2020, 3:50 AM
samparker added a reviewer: efriedma.
samparker updated this revision to Diff 286529.Aug 19 2020, 3:59 AM
samparker retitled this revision from [WIP] [ARM] Make MachineVerifier more strict about terminators to [ARM] Make MachineVerifier more strict about terminators.
samparker edited reviewers, added: dmgreen, yroux, SjoerdMeijer; removed: rengolin, javed.absar.
samparker changed the repository for this revision from rL LLVM to rG LLVM Github Monorepo.

Dragged this patch in 2020 and the original concerns around inefficient codegen now appear to be gone. Most of these changes are MIR test changes where I've had to insert a block after a tBX_RET, but I've also add to regenerate a few MIR tests completely to save my sanity.

Herald added a project: Restricted Project. · View Herald Transcript
yroux added a comment.Aug 19 2020, 9:32 AM

I've tested the patch and it fixes the liveness issue seen with Machine Outliner enabled.

The patch LGTM but maybe needs another review

efriedma added inline comments.Aug 19 2020, 1:48 PM
llvm/test/CodeGen/Thumb2/mve-satmul-loops.ll
1141

This is an instance of the problem I was running into with the original patch.

samparker updated this revision to Diff 286767.Aug 20 2020, 3:54 AM

Thanks both. I think I've got rid of the pesky branches.

  • All return instructions are now not analysable.
  • The block iterators have been changed within analyzeBranch so we can look at each instruction within a bundle.
  • If the last instruction in the block is an unpredicated unconditional branch, that targets the layout successor, we remove the branch.
yroux added a comment.Aug 25 2020, 8:34 AM

Hi,

I think a few more testcases need to be updated:

CodeGen/Thumb2/LowOverheadLoops/mve-float-loops.ll
CodeGen/Thumb2/mve-postinc-distribute.ll
CodeGen/Thumb2/mve-postinc-lsr.ll
CodeGen/Thumb2/mve-selectcc.ll
Profile-armhf :: instrprof-lto-pgogen.c

samparker updated this revision to Diff 287688.Aug 25 2020, 9:13 AM

Hi @yroux , I've rebased and updated test/CodeGen/Thumb2/mve-selectcc.ll test, but the others that you mentioned seemed fine. Was there particular parts of those tests that you think should change?

yroux added a comment.Aug 26 2020, 1:37 AM

Hi Sam,

I've tested the patch on top of 4d90ff5 rev on a native ARM plateform the issues I see are:

Thumb2/LowOverheadLoops/mve-float-loops.ll:908:15: error: CHECK-NEXT: expected string not found in input
; CHECK-NEXT: b .LBB4_6

^

<stdin>:511:1: note: scanning from here
.LBB4_6: @ %for.body.preheader11

Thumb2/mve-postinc-lsr.ll:41:15: error: CHECK-NEXT: expected string not found in input
; CHECK-NEXT: b .LBB0_6

^

<stdin>:62:1: note: scanning from here
.LBB0_6: @ %for.body.preheader12

Thumb2/mve-postinc-distribute.ll:307:15: error: CHECK-NEXT: expected string not found in input
; CHECK-NEXT: b .LBB2_6

^

<stdin>:181:1: note: scanning from here
.LBB2_6: @ %for.body.preheader12

I'll try to find what is wrong with compiler-rt/test/profile/instrprof-lto-pgogen.c and let you know

Hi Yvan,

Could you post the output of those failing tests so I can compare them fully? I'm on an X86 machine but I hope that isn't making a difference...
Thanks.

yroux added a comment.Aug 26 2020, 6:51 AM

My bad, I applied the previous version where a branch was inserted in those tests but your last version is fine.

I'm ok with the patch

Thanks for confirming. What do you think @efriedma?

This revision is now accepted and ready to land.Aug 26 2020, 1:50 PM
llvm/test/CodeGen/Thumb2/LowOverheadLoops/dont-remove-loop-update.mir