This patch transforms the Scheduler class into the ExecuteStage. Most of the logic remains.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
tools/llvm-mca/Backend.cpp | ||
---|---|---|
48 ↗ | (On Diff #148157) | How do you intend to make this independent of the actual type of the Stages in the long term (when we have a vector<std::unique_ptr<Stage>> instead of Dispatch and Execute) ? |
Hi Matt,
I am not convinced that it is a good idea to convert the Scheduler into a Stage.
I think the "execute stage" should just use/interact with the hardware schedulers to orchestrate the execution.
The statge would be also responsible for notifying events to listeners.
What I am trying to say is that the stage should only be responsible for the process (i.e. orchestrating the execution).
The logic that lives in Scheduler::cycleEvent() is essentially where the "process" logic is implemented. That logic (plus the event notification logic) should be moved to the stage class. The rest of the logic should still be part of the Scheduler class. Basically, the stage is only where we control the execution. It helps decoupling the actual feature from the process (i.e. how we use the feature/interact with the hardware components).
This is an updated patch that maintains the logic of the Scheduler, but decoupling the hardware component from the execution component.
The ExecuteStage drives the Scheduler and provides the notification functionality.
- Scheduler::cycleEvent is now ExecuteStage::preExecute
- Scheduler::scheduleInstruction is now ExecuteStage::execute
- Scheduler::canBeDispatched now returns a HWStallEvent (or NoStall) instead of a bool. We might want to rename this routine to something more appropriate, it makes more sense to name the routine something that does not imply a boolean return value. I left the name the same for this patch.
- The Backend pointer (Owner) in ExecuteStage will eventually go away once we register the listeners to the stages.
Hi Matt
thanks for the patch. Sorry for the late review; it took me a while to go through all of its parts (I was not expecting so many changes).
I have a few nits (see my comments below):
tools/llvm-mca/ExecuteStage.cpp | ||
---|---|---|
30 ↗ | (On Diff #150568) | Something like a small paragraph with a brief overview of the sequence of steps performed by this method would really help. I know that it was missing in the original implementation. However, I think it really helps a lot if we add a small description here. Some steps could be moved to a separate function. That might help with making the code more readable (if we choose good names for functions). |
78 ↗ | (On Diff #150568) | Please add a small comment here. |
tools/llvm-mca/HWEventListener.h | ||
106–107 ↗ | (On Diff #150568) | See my other comment. |
tools/llvm-mca/Scheduler.cpp | ||
235–236 ↗ | (On Diff #150568) | Could you change this method so that it returns a bool? I don't particularly like the fact that , as a consequence of this change, we now need a new value in the GenericEventType enum. |
tools/llvm-mca/Scheduler.h | ||
462 ↗ | (On Diff #150568) | Emphasize that this method is meant to be called when a new cycle starts. |
497–499 ↗ | (On Diff #150568) | You can reuse the previous NDEBUG block. I think you should remove the #endif at line 497 instead of opening a new block. |
tools/llvm-mca/Scheduler.cpp | ||
---|---|---|
235–236 ↗ | (On Diff #150568) | Sounds good! Alternatively, we could make canBeDispatched return Optional<HWStallEvent::GenericEventType>, but I'm happy with an in/out in this case, simple enough. |