This is an archive of the discontinued LLVM Phabricator instance.

[MBP] Avoid placing random blocks between loop preheader and header
ClosedPublic

Authored by reames on Mar 2 2016, 3:05 PM.

Details

Summary

If we have a loop with a rarely taken path, we will prune that from the blocks which get added as part of the loop chain. The problem is that we weren't then recognizing the loop chain as schedulable when considering the preheader when forming the function chain. We'd then fall to various non-predecessors before finally scheduling the loop chain (as if the CFG was unnatural.) The net result was that there could be lots of garbage between a loop preheader and the loop, even though we could have directly fallen into the loop. It also meant we separated hot code with regions of colder code.

The particular reason for the rejection of the loop chain was that we were scanning predecessor of the header, seeing the backedge, believing that was a globally more important predecessor (true), but forgetting to account for the fact the backedge precessor was already part of the existing loop chain (oops!.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 49673.Mar 2 2016, 3:05 PM
reames retitled this revision from to [MBP] Avoid placing random blocks between loop preheader and header.
reames updated this object.
reames added reviewers: davidxl, congh, djasper, chandlerc.
reames added a subscriber: llvm-commits.
reames added inline comments.Mar 2 2016, 3:08 PM
lib/CodeGen/MachineBlockPlacement.cpp
210 ↗(On Diff #49673)

As a side note, the name of this variable is highly misleading. When scheduling function chains, this is used to track unscheduled predecessors outside of any loop. Once this change is in, I'd like to rename this to something more meaningful. Any suggestions?

chandlerc accepted this revision.Mar 2 2016, 3:16 PM
chandlerc edited edge metadata.

Gah, excellent bug fix. That makes perfect sense to me, and thanks!

lib/CodeGen/MachineBlockPlacement.cpp
210 ↗(On Diff #49673)

No real idea. =/ I agree, it was bad when it started and it has gotten worse.

This revision is now accepted and ready to land.Mar 2 2016, 3:16 PM
congh accepted this revision.Mar 2 2016, 4:00 PM
congh edited edge metadata.

Great fix! Then we can ignore any predecessors in the same chain and treat the chain as a single node, in which case the chain should always be a loop chain.

lib/CodeGen/MachineBlockPlacement.cpp
210 ↗(On Diff #49673)

Maybe ChainPredecessors? But in loops this may also be misleading. If we treat the function as a loop body, maybe ChainPredecessorsInLoop is better?

davidxl edited edge metadata.Mar 2 2016, 4:05 PM

LGTM.

test/CodeGen/X86/mbp-false-cfg-break.ll
9 ↗(On Diff #49673)

There does not seem to be a guarantee about the order between rare and rare1. How about using CHECK-NOT to make sure cold blocks are intermixed with hot ones?

This revision was automatically updated to reflect the committed changes.
reames added inline comments.Mar 2 2016, 4:08 PM
lib/CodeGen/MachineBlockPlacement.cpp
210 ↗(On Diff #49673)

How does UnscheduledPredecessors sound? I'm going for something that describes how the value changes over time and what that actually means.

congh added inline comments.Mar 2 2016, 4:40 PM
lib/CodeGen/MachineBlockPlacement.cpp
210 ↗(On Diff #49673)

Sounds good!

davidxl added a subscriber: davidxl.Mar 2 2016, 4:45 PM

Or perhaps UnscheduledPredInLoop ?

David

reames added a subscriber: reames.Mar 2 2016, 4:47 PM

I'm am specifically trying to avoid any mention of loops because the
variable is used when not in any loop.

Philip

reames added a comment.Mar 2 2016, 5:03 PM

Landed in 262571