Page MenuHomePhabricator

[ScheduleDAG] Don't schedule node with physical register interference
ClosedPublic

Authored by efriedma on Jun 1 2017, 6:28 PM.

Details

Summary

https://reviews.llvm.org/D31536 didn't really solve the problem it was trying to solve; it got rid of the assertion failure, but we were still scheduling the DAG incorrectly (mixing together instructions from different calls), leading to a MachineVerifier failure.

In order to schedule the DAG correctly, we have to make sure we don't schedule a node which should be blocked by an interference. Fix ScheduleDAGRRList::PickNodeToScheduleBottomUp so it doesn't pick a node like that.

The added call to FindAvailableNode() is the key change here; this makes sure we don't try to schedule a call while we're in the middle of scheduling a different call. I'm not sure this is the right approach; in particular, I'm not sure how to prove we don't end up with an infinite loop of repeatedly backtracking.

This also reverts the code change from D31536. It doesn't do anything useful: we should never schedule an ADJCALLSTACKDOWN unless we've already scheduled the corresponding ADJCALLSTACKUP.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Jun 1 2017, 6:28 PM
javed.absar added inline comments.Jun 7 2017, 4:06 PM
lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
552 ↗(On Diff #101143)

FindCallSeqStart - line 476 : assert(Best); already checks, so this assertion seems redundant

efriedma added inline comments.Jun 7 2017, 4:43 PM
lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
552 ↗(On Diff #101143)

That's true along that particular codepath, but FindCallSeqStart can return null along other codepaths; the assertion isn't obviously redundant.

samparker edited edge metadata.Jun 9 2017, 5:07 AM

Hi Eli,

Thanks for taking a look at this again, but I don't think I can be of much help with my very limited understanding of this code. I have no idea if you're going to get stuck backtracking, hopefully Andrew can shine some light.

cheers,
sam

atrick accepted this revision.Jul 20 2017, 2:02 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Jul 20 2017, 2:02 PM
This revision was automatically updated to reflect the committed changes.