Conversation
| void print_help(const string prog_name) | ||
| void print_help(const string & progname) | ||
| { | ||
| cerr << "\n Usage: \n " << progname |
There was a problem hiding this comment.
Can you merge this change into the previous change? I would like each change to compile on its own and it is clear that this cannot compile on its own and needs both changes
There was a problem hiding this comment.
the change of the parameter name is not necessary and requires changing the name in other code lines. Could you please keep the old name?
| void print_help(const string prog_name) | ||
| void print_help(const string & progname) | ||
| { | ||
| cerr << "\n Usage: \n " << progname |
There was a problem hiding this comment.
Can you merge this change into the previous change? I would like each change to compile on its own and it is clear that this cannot compile on its own and needs both changes
There was a problem hiding this comment.
same here. Please keep the old name.
opcm
left a comment
opcm
left a comment
There was a problem hiding this comment.
thanks for the patch. Please implement the requested changes.
| auto ctrl = pmu.counterControl[c]; | ||
| if (ctrl.get() != nullptr) | ||
| { | ||
| *ctrl = MC_CH_PCI_PMON_CTL_EN; |
There was a problem hiding this comment.
I would prefer if you keep " *ctrl = MC_CH_PCI_PMON_CTL_EN;" because IIRC some PMU docs suggested that the PMU control register needs to be written without the event/umask and then in the next write the event/umask needs to be written.
| void print_help(const string prog_name) | ||
| void print_help(const string & progname) | ||
| { | ||
| cerr << "\n Usage: \n " << progname |
There was a problem hiding this comment.
the change of the parameter name is not necessary and requires changing the name in other code lines. Could you please keep the old name?
| void print_help(const string prog_name) | ||
| void print_help(const string & progname) | ||
| { | ||
| cerr << "\n Usage: \n " << progname |
There was a problem hiding this comment.
same here. Please keep the old name.
| } | ||
| if (m->localMemoryRequestRatioMetricAvailable()) { | ||
| cout << " LOCAL : ratio of local memory requests to memory controller in %\n"; | ||
| cout << "LLCRDMISSLAT: average latency of last level cache miss for reads and prefetches (in ns)\n"; |
There was a problem hiding this comment.
the availability of LLCRDMISSLAT metric should be checked with m->LLCReadMissLatencyMetricsAvailable() and not with m->localMemoryRequestRatioMetricAvailable()
|
@GermanAizek ping |
3 participants
@rdementi,
Check my changes, thanks.
Feedback with me with comments.