This is an archive of the discontinued LLVM Phabricator instance.

[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
604–605

Doc comment (///)?

609

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

716

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
1834

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
604–605

typo.

716

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

llvm/lib/CodeGen/MachineScheduler.cpp
1834

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
1834

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.EditedApr 28 2019, 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 ...

Gentle ping ... Thank you!

fhahn added a comment.Jun 17 2019, 3:43 AM

Thanks for the update. I think it's better structured now for preserving the relevant state. It would be good to see if @atrick and @MatzeB are happy with that direction as well.

llvm/lib/CodeGen/MachineScheduler.cpp
1834

I guess this could go into the header.

1919–1920

I guess with this change, it would also make sense to use the getXXX functions here and elsewhere, instead of manually getting them from State.

steven.zhang marked 3 inline comments as done.

Address reviewer's comments.

  • Move the ctor to the header.
  • Declare the SchedState as "class" instead of "struct" (Is it ok?).
  • Rebase the patch to master and resolve the conflict.
steven.zhang added inline comments.Jun 17 2019, 10:52 PM
llvm/lib/CodeGen/MachineScheduler.cpp
1834

ok

1919–1920

The State will be read and updated by SchedBoundary. i.e.

RetiredMOps += IncMOps

If we want to have getXXX, we have to provide another setXXX, and my understanding is that, it is an internal data structure for SchedBoundary. Maybe, we could declare it as class, and it is a friend class of SchedBoundary ?

jonpa added a comment.Sep 16 2022, 7:52 AM

please rebase

Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 7:52 AM
Herald added a subscriber: ecnelises. · View Herald Transcript