This is an archive of the discontinued LLVM Phabricator instance.

Reset the TopRPTracker's position in ScheduleDAGMILive::initQueues
ClosedPublic

Authored by kparzysz on Apr 22 2016, 2:56 PM.

Details

Summary

ScheduleDAGMI::initQueues changes the RegionBegin to the first non-debug instruction. Since it does not track register pressure, it does not affect any RP trackers. ScheduleDAGMILive inherits initQueues from ScheduleDAGMI, and it does reset the TopTPTracker in its schedule method. Any derived, target-specific scheduler will need to do it as well, but the TopRPTracker is only exposed as a "const" object to derived classes. Without the ability to modify the tracker directly, this leaves a derived scheduler with a potential of having the TopRPTracker out-of-sync with the CurrentTop.

The symptom of the problem:

void llvm::ScheduleDAGMILive::scheduleMI(llvm::SUnit *, bool): Assertion `TopRPTracker.getPos() == CurrentTop && "out of sync"' failed.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz updated this revision to Diff 54732.Apr 22 2016, 2:56 PM
kparzysz retitled this revision from to Reset the TopRPTracker's position in ScheduleDAGMILive::initQueues.
kparzysz updated this object.
kparzysz added a reviewer: atrick.
kparzysz set the repository for this revision to rL LLVM.
kparzysz added a subscriber: llvm-commits.

I don't know if this fix is the best idea, I can change it to whatever else as long as the problem is fixed.

MatzeB edited edge metadata.Apr 28 2016, 11:13 AM

You should be able to access the "protected:" TopRPTracker field in subclasses directly. Regardless this change should be fine.

Is the test minimal yet? It appears to be a lot bigger than necessary (all the metadata debug info seems superfluous for example).

include/llvm/CodeGen/MachineScheduler.h
467–471 ↗(On Diff #54732)

This is not performancce critical so the implementation can stay in the .cpp file.

The test is close to minimal. The debug information is necessary, since it is the presence of DBG_VALUE that causes the beginning of the scheduling region to be moved. This was the first thing I tried to remove that the test passed without it.

kparzysz updated this revision to Diff 55449.Apr 28 2016, 11:47 AM
kparzysz edited edge metadata.

Moved initQueues to the .cpp file.

kparzysz marked an inline comment as done.Apr 28 2016, 11:48 AM
MatzeB accepted this revision.Apr 28 2016, 11:49 AM
MatzeB edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 28 2016, 11:49 AM
This revision was automatically updated to reflect the committed changes.