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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: include/llvm/MC/MCSchedule.h:195
+    // The name of the ProcResource that this counter measures.
+    const char *const ProcResName;
   };
----------------
What would `ProcResName` be is case of retired instruction for instance?


================
Comment at: include/llvm/MC/MCSchedule.h:203
+  static const MCPfmCountersInfo &getDefault() { return Default; }
+  static const MCPfmCountersInfo Default;
 };
----------------
Why do you need both?


================
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,
----------------
I don't understand the error message.


================
Comment at: utils/TableGen/CodeGenSchedule.cpp:778
+      PrintFatalError(AliasRW.TheDef->getLoc(),
+                      "Multiple aliases "
+                      "defined for processor " +
----------------
join string literals and reapply formatting.


================
Comment at: utils/TableGen/CodeGenSchedule.cpp:884
+      PrintFatalError(Inst->TheDef->getLoc(),
+                      "Instruction's sched class "
                       "must not be subtarget specific.");
----------------
Same here


================
Comment at: utils/TableGen/CodeGenSchedule.cpp:1204
     }
-    assert(SchedClasses.size() < (NumInstrSchedClasses*6) &&
+    assert(SchedClasses.size() < (NumInstrSchedClasses * 6) &&
            "too many SchedVariants");
----------------
Can you explain what the 6 is referring to?


================
Comment at: utils/TableGen/CodeGenSchedule.cpp:1431
     if (ProcIndices[0] && Variant.ProcIdx) {
-      unsigned Cnt = std::count(ProcIndices.begin(), ProcIndices.end(),
-                                Variant.ProcIdx);
+      unsigned Cnt =
+          std::count(ProcIndices.begin(), ProcIndices.end(), Variant.ProcIdx);
----------------
const


================
Comment at: utils/TableGen/CodeGenSchedule.cpp:1462
+    PrintFatalError(SchedRW.TheDef->getLoc(),
+                    "No variant of this type has "
                     "a matching predicate on any processor");
----------------
join string literals


================
Comment at: utils/TableGen/CodeGenSchedule.cpp:1948
+    errs()
+        << "\n\nIncomplete schedule models found.\n"
+        << "- Consider setting 'CompleteModel = 0' while developing new "
----------------
Can you reformat


================
Comment at: utils/TableGen/SubtargetEmitter.cpp:1892
   OS << '\n'; OS.indent(22);
-  OS << Target << "ProcSchedKV, "
-     << Target << "WriteProcResTable, "
-     << Target << "WriteLatencyTable, "
-     << Target << "ReadAdvanceTable, ";
+  OS << Target << "ProcSchedKV, " << Target << "ProcPfmKV, " << Target
+     << "WriteProcResTable, " << Target << "WriteLatencyTable, " << Target
----------------
llvm::format would help here
```
OS << llvm::format("{0}ProcSchedKV, {0}ProcPfmKV, {0}WriteProcResTable, {0}WriteLatencyTable, {0}ReadAdvanceTable, ", Target);
```


================
Comment at: utils/TableGen/SubtargetEmitter.cpp:1986
   OS << '\n'; OS.indent(24);
-  OS << Target << "ProcSchedKV, "
-     << Target << "WriteProcResTable, "
-     << Target << "WriteLatencyTable, "
-     << Target << "ReadAdvanceTable, ";
+  OS << Target << "ProcSchedKV, " << Target << "ProcPfmKV, " << Target
+     << "WriteProcResTable, " << Target << "WriteLatencyTable, " << Target
----------------
Same here.


Repository:
  rL LLVM

https://reviews.llvm.org/D52932