[PATCH] D52932: [MCSched] Bind PFM Counters to the CPUs instead of the SchedModel.

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

@gchatelet a bunch of your comments are on old code touched by unwanted reformatting. I ignored these since I have reverted the formatted changes.



================
Comment at: include/llvm/MC/MCSchedule.h:195
+    // The name of the ProcResource that this counter measures.
+    const char *const ProcResName;
   };
----------------
gchatelet wrote:
> What would `ProcResName` be is case of retired instruction for instance?
the retired counter is a scheduler concept, not tied to proc resources. It would go with `CycleCounter` and `UopsCounter` above.


================
Comment at: include/llvm/MC/MCSchedule.h:203
+  static const MCPfmCountersInfo &getDefault() { return Default; }
+  static const MCPfmCountersInfo Default;
 };
----------------
gchatelet wrote:
> Why do you need both?
For consistency with MCSchedModel (see getValueForCpu() below).


================
Comment at: lib/MC/MCSchedule.cpp:152
+static_assert(std::is_pod<MCPfmCountersInfo>::value,
+              "We shouldn't have a static constructor here");
+const MCPfmCountersInfo MCPfmCountersInfo::Default = {nullptr, nullptr,
----------------
gchatelet wrote:
> I don't understand the error message.
This is ensuring that the value can be constructed by the linker without running any dynamic initialization.


Repository:
  rL LLVM

https://reviews.llvm.org/D52932