[PATCH] D98628: [MCA] Disable RCU for InOrderIssueStage

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

I personally don't like the idea of making the RCU optional in the RetireStage. The is no point in having a RetireStage if there is no RCU.

For in-order processors, the RetireStage is not really needed. Instructions are always issued in-order, and for some instructions, we allow out-of-order commit.
For the purpose of MCA, we don't require an RCU; the retire stage would simply propagates "instruction retired" events. Having a stage just for that purpouse is a bit silly.

If you want to fully get rid of the RCU, then there is a better way: just get rid of the RetireStage for in-order processors, and generate "instruction retired" events directly from the InOrderIssueStage.
For in-order, the "instruction executed" and "instruction retired" events always match (both events are generated during the same cycle).

I suggest the following changes:

1. Remove the RetireStage and always notify that an instruction is retired after it has finished execution.
2. (As a consequence of 1.) You don't need any changes to the RetireStage. The RCU should not be optional, so pass it always by reference to the constructor.
3. Please revert the changes to the RetireStatistics. We don't use those stats for in-order processors, so there is no reason why that file should be touched.
4. (As a consequence of 1. and 2.) Do not print character `R` in the TimelineView if `E` and `R` are the same cycle. This means ignoring a loop at around line 244.

Point 4. means that we no longer get the `R` character in the timeline. In my opinion this is more accurate, since focus is on the write-back, and there is no delayed commit. We could add a note about this in the section which describes the timeline view.

Minor nit:
Please rename function `notifyInstructionExecute` (in InOrderIssueStage.cpp) to `notifyInstructionIssue`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98628/new/

https://reviews.llvm.org/D98628