Page MenuHomePhabricator

[NFC] Add SchedState to allow forwarding the Scheduling state between MBB
Needs ReviewPublic

Authored by steven.zhang on Mar 17 2019, 11:01 PM.

Details

Summary

This is a NFC patch to improve the Generic scheduler to allow forwarding the scheduling state between MBB. A new class named SchedState is added to keep all the scheduling state that could be forwarded to next MBB.
Later patch would be committed to leverage this to improve the scheduler for some target.

If MBB2 has single pred MBB1, the MBB1's scheduler state could be forwarded to MBB2 to model the scheduler more precise.

Diff Detail

Event Timeline

steven.zhang created this revision.Mar 17 2019, 11:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2019, 11:01 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn added a subscriber: jonpa.Mar 18 2019, 4:40 AM

On a first look, the patch looks useful! Any chance to provide the follow-up patch that uses it as well? that would be helpful to see the bigger picture. Also, are you seeing any benefits by preserving the state?

llvm/include/llvm/CodeGen/MachineScheduler.h
615

Doc comment (///)?

620

Why do we need an explicit assignment operator? Does the default one not work?

714

Why not have getState() which returns const SchedState& and setState? This way, if we need a copy in the caller, it needs to be explicit.

I think it would also be better to do the restore at the point where we initialize the ScheduleBoundary, rather than providing a restore helper. Maybe have SchedBoundary take a SchedState object, which defaults to SchedState()?

llvm/lib/CodeGen/MachineScheduler.cpp
1853

Personally, I think it would be simpler to have the constructor create a 'resetted' object. In SchedBoundary::reset(), we then can either assign a 'resetted' object or a user provided one.

jsji added a reviewer: jsji.Mar 18 2019, 8:12 AM
jonpa added a subscriber: uweigand.Mar 18 2019, 2:09 PM

I did something similar for the SystemZ PostRA scheduler. Is this patch taking that into consideration? I have not looked at this in detail, but it would be nice if the SystemZ implementation could use this or at least parts of it...

steven.zhang added a comment.EditedMar 18 2019, 7:03 PM

I did something similar for the SystemZ PostRA scheduler. Is this patch taking that into consideration? I have not looked at this in detail, but it would be nice if the SystemZ implementation could use this or at least parts of it...

Yes, I will take this into consideration. Thank you for the information.

steven.zhang added a comment.EditedMar 18 2019, 7:08 PM

On a first look, the patch looks useful! Any chance to provide the follow-up patch that uses it as well? that would be helpful to see the bigger picture. Also, are you seeing any benefits by preserving the state?

I believe SystemZ has already seen some benefit basing on this, as it seems that, they have already committed some patches in SystemZ target to preserve the state. There will be 4 patches, including this one. 1 NFC patch to extend the MI scheduler to support the state forward to next MBB. 1 NFC patch for SystemZ to adopt the new infrastructure. And 1 new patch to enable this feature for PowerPC. I haven't seen the benefit from the spec2017 yet on PowerPC , and I am still investigating, In theory, we should have some benefit as the resource is modeled more precise.

steven.zhang marked 3 inline comments as done.

Address comments.

llvm/include/llvm/CodeGen/MachineScheduler.h
615

typo.

714

Sounds good, and that is exactly what I want to do.

llvm/lib/CodeGen/MachineScheduler.cpp
1853

Sounds good. I will leave the way how to reset/save/store the state into next NFC infrastructure patch if this one is accepted.

steven.zhang marked 2 inline comments as done.Mar 18 2019, 8:20 PM

gentle ping ...

jsji added a reviewer: jonpa.Mar 25 2019, 6:53 AM
fhahn added inline comments.Mar 26 2019, 2:58 PM
llvm/lib/CodeGen/MachineScheduler.cpp
1853

I think it would be helpful if you could post your follow-up patches on Phabricator as well, and link them. That would make it easier to see the full picture.

ok. I will prepare another two patches first, so that, we could continue the review.

steven.zhang added a comment.EditedSun, Apr 28, 8:02 PM

@fhahn I have completed all the needed patches, and it would be great if you have the time to continue the review. This is the whole picture.
https://reviews.llvm.org/D61248 is the 1st patch to allow the schedule strategy to forward the schedule state between MBB.
https://reviews.llvm.org/D61249 is the 2nd patch to update the schedule strategy for SystemZ target to adapt with 1st patch.
https://reviews.llvm.org/D59480 (this patch) is the 3rd patch to add the scheduled state data structure, so that, it could be kept somewhere.
https://reviews.llvm.org/D61250 is the last patch to forward the scheduled state 3rd patch added for PostGenericScheduler and enable it for PowerPC target.

Gentle ping ...