This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Add the RetireStage.
ClosedPublic

Authored by mattd on May 22 2018, 8:25 PM.

Details

Summary

This class maintains the same logic as the original RetireControlUnit.

This is just an intermediate patch to make the RCU a Stage. Future patches will remove the dependency on the DispatchStage, and then more properly populate the pre/execute/post Stage interface.

Diff Detail

Repository
rL LLVM

Event Timeline

mattd created this revision.May 22 2018, 8:25 PM
courbet accepted this revision.May 22 2018, 11:16 PM
This revision is now accepted and ready to land.May 22 2018, 11:16 PM
andreadb accepted this revision.May 23 2018, 2:30 AM

A couple of minor nits. Otherwise it LGTM too.

tools/llvm-mca/Backend.cpp
41 ↗(On Diff #148152)

Backend.cpp should not be modified by this patch. Please get rid of this line diff.

tools/llvm-mca/DispatchStage.h
11–12 ↗(On Diff #148152)

Can you improve the description? The old description is no longer valid.

tools/llvm-mca/RetireStage.h
88 ↗(On Diff #148152)

This comment can be removed as it doesn't add much.

On a second thought,
wouldn't it be better if we still keep the RetireControlUnit abstraction?
We can have that the RetireStage acts as a proxy for the RetireControlUnit.

Basically, the logic in RetireStage::cycleEvent() and onInstructionExecuted() should be implemented by the "stage".
While, the actual RetireControlUnit functionality should live in its own class. I think it logically helps to distinguish between the actual pipeline process from the implementation of the RCU. It also allows the Stage to not be bound to a specific implementation of the RCU.

mattd updated this revision to Diff 148532.May 24 2018, 6:03 PM

I like Andrea's suggestion, and that we still maintain the RCU as a separate component.

This patch maintains that separation, but places ownership in the Backend. Additionally, the RegisterFile (renamed to RF instead of RAT) is also another simulated hardware component that is owned by the Backend. Any other MCA component that requires the use of such hardware abstraction will be passed pointers to those items during construction. The Owner pointer in the RetireStage (sorry) will hopefully disappear once we move the eventing (notification) logic from solely being owned by the Backend, into the the Stages.

This patch moves the notifyInstructionRetired and cycleEvent from the RCU into the RetireStage.

mattd planned changes to this revision.May 24 2018, 6:03 PM
mattd retitled this revision from [llvm-mca] Rename the RetireControlUnit into RetireStage. NFC. to [llvm-mca] Add the RetireStage. .
mattd requested review of this revision.May 24 2018, 6:06 PM
This revision is now accepted and ready to land.May 24 2018, 6:06 PM
mattd requested review of this revision.May 24 2018, 6:06 PM

I like Andrea's suggestion, and that we still maintain the RCU as a separate component.

This patch maintains that separation, but places ownership in the Backend. Additionally, the RegisterFile (renamed to RF instead of RAT) is also another simulated hardware component that is owned by the Backend. Any other MCA component that requires the use of such hardware abstraction will be passed pointers to those items during construction.

Nice patch. Thanks!

The Owner pointer in the RetireStage (sorry) will hopefully disappear once we move the eventing (notification) logic from solely being owned by the Backend, into the the Stages.

No problem. One step at the time :-).

This patch moves the notifyInstructionRetired and cycleEvent from the RCU into the RetireStage.

LGTM modulo a few nits (see comments below).

Thanks
-Andrea

tools/llvm-mca/DispatchStage.cpp
33 ↗(On Diff #148532)

s/checkRF/checkPRF

tools/llvm-mca/DispatchStage.h
62–63 ↗(On Diff #148532)

These should be references.
Also, could you please rename RF --> PRF (as in physical register file)?

92–93 ↗(On Diff #148532)

Pass the RCU and PRF by reference to the constructor of DispatchStage.

tools/llvm-mca/RetireStage.h
31–32 ↗(On Diff #148532)

Make these references.
Please rename RF into PRF.

tools/llvm-mca/Scheduler.h
405 ↗(On Diff #148532)

Make this a reference.

andreadb accepted this revision.May 25 2018, 4:52 AM
This revision is now accepted and ready to land.May 25 2018, 4:52 AM
mattd updated this revision to Diff 148625.May 25 2018, 10:28 AM
  • Renamed RegisterFile references from RF to PRF.
  • I think it's simplest to make RCU and PRF complete objects instead of pointers or unique_ptrs.
  • For now RCU and PRF are owned by the pipeline abstraction: Backend.
  • RCU and PRF are now passed as references to Backend components that need them.
andreadb accepted this revision.May 25 2018, 10:35 AM

LGTM. Thanks!

This revision was automatically updated to reflect the committed changes.