[PATCH] D44309: [llvm-mca] Revactor event listeners to make the backend agnostic to event types.
Andrea Di Biagio via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 9 09:47:03 PST 2018
andreadb added a comment.
Thanks Clement.
Please run clang-format on the patch. I found that the code was not correctly formatted in several places.
See my comments below.
================
Comment at: tools/llvm-mca/Backend.h:85
unsigned getNumInstructions() const { return SM.size(); }
+ const Instruction& getInstruction(unsigned Index) const { return *Instructions.find(Index)->second; }
+ void eraseInstruction(unsigned Index) { Instructions.erase(Index); }
----------------
80-col.
================
Comment at: tools/llvm-mca/BackendStatistics.cpp:24-36
+ switch (Event.Type) {
+ case HWInstructionEvent::Retired:
+ ++NumRetired;
+ break;
+ case HWInstructionEvent::Issued:
+ ++NumIssued;
+ break;
----------------
clang-format.
================
Comment at: tools/llvm-mca/BackendStatistics.h:120-121
void printView(llvm::raw_ostream &OS) const override {
- printDispatchStalls(OS, B.getNumRATStalls(), B.getNumRCUStalls(),
- B.getNumSQStalls(), B.getNumLDQStalls(),
- B.getNumSTQStalls(), B.getNumDispatchGroupStalls());
+ printDispatchStalls(OS, B.getNumRATStalls(), B.getNumRCUStalls(), B.getNumSQStalls(),
+ B.getNumLDQStalls(), B.getNumSTQStalls(), B.getNumDispatchGroupStalls());
printRATStatistics(OS, B.getTotalRegisterMappingsCreated(),
----------------
Remove this change. Alternatively, run clang-format on this file.
================
Comment at: tools/llvm-mca/BackendStatistics.h:122-123
+ B.getNumLDQStalls(), B.getNumSTQStalls(), B.getNumDispatchGroupStalls());
printRATStatistics(OS, B.getTotalRegisterMappingsCreated(),
- B.getMaxUsedRegisterMappings());
+ B.getMaxUsedRegisterMappings());
printDispatchUnitStatistics(OS);
----------------
clang-format.
================
Comment at: tools/llvm-mca/Dispatch.cpp:154-160
+ DEBUG(dbgs() << "[E] Instruction Dispatched: " << Index << '\n');
+ Owner->notifyInstructionEvent(HWInstructionEvent(HWInstructionEvent::Dispatched, Index));
}
void DispatchUnit::notifyInstructionRetired(unsigned Index) {
- Owner->notifyInstructionRetired(Index);
+ DEBUG(dbgs() << "[E] Instruction Retired: " << Index << '\n');
+ Owner->notifyInstructionEvent(HWInstructionEvent(HWInstructionEvent::Retired, Index));
----------------
80-col.
================
Comment at: tools/llvm-mca/HWEventListener.h:24
-class HWEventListener {
+// An event
+class HWInstructionEvent {
----------------
We need a better (doxygen) comment here.
================
Comment at: tools/llvm-mca/HWEventListener.h:27
public:
- // Events generated by the Retire Control Unit.
- virtual void onInstructionRetired(unsigned Index) {};
+ // FIXME: This should be subtarget-specific. Move to TD files.
+ enum EventType {
----------------
Could you please explain what is the long term goal? Do you plan to remove this EventType?
================
Comment at: tools/llvm-mca/HWEventListener.h:42
+
+ // The event type. The exact meaning depends on the subtarget,
+ const unsigned Type;
----------------
Period at end of sentence.
================
Comment at: tools/llvm-mca/ResourcePressureView.cpp:48-50
+ if (Event.Type != HWInstructionEvent::Issued) {
+ return;
+ }
----------------
You don't need braces here.
================
Comment at: tools/llvm-mca/ResourcePressureView.h:62
#include "View.h"
+#include "SourceMgr.h"
#include "llvm/MC/MCInstPrinter.h"
----------------
Why this change?
================
Comment at: tools/llvm-mca/Scheduler.h:457-458
+
+ // The dispatch unit gets notified when instructions are executed.
+ DispatchUnit *DU;
----------------
Is the plan to remove this dependency in the long term?
Repository:
rL LLVM
https://reviews.llvm.org/D44309
More information about the llvm-commits
mailing list