[llvm-exegesis] Fix PfmIssueCountersTable creation

[RKSimon]

Summary

This patch ensures that the pfm issue counter tables are the correct size, accounting for the invalid resource entry at the beginning of the resource tables.

It also fixes an issue with pfm failing to match event counters due to a trailing comma added to all the event names.

I've also added a counter comment to each entry as it helps locate problems with the tables.

Note: I don't have access to a SandyBridge test machine, which is the only model to make use of multiple event counters being mapped to a single resource. I don't know if pfm accepts a comma-seperated list or not, but that is what it was doing.

[gchatelet]

FYI Clement is OOO so I'll take over the review.
I'm not too familiar with TableGen: do you have a snippet of the generated code?

[andreadb]

Hi Guillaume,

this is an example of what we currently get in tablegen.

static const char* HaswellModelPfmIssueCounters[] = {
  nullptr,
  nullptr,
  nullptr,
  "uops_dispatched_port:port_0,",  //HWPort0
  "uops_dispatched_port:port_1,",  //HWPort1
  "uops_dispatched_port:port_2,",  //HWPort2
  "uops_dispatched_port:port_3,",  //HWPort3
  "uops_dispatched_port:port_4,",  //HWPort4
  "uops_dispatched_port:port_5,",  //HWPort5
  "uops_dispatched_port:port_6,",  //HWPort6
  "uops_dispatched_port:port_7,",  //HWPort7
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
  nullptr,
};

You can see the comma at the end of each string.

Also, we found that the number of entries in that table doesn't actually
match the number of processor resource unit entries. There should be 24
entries (there are 23).
When initializing that table, you weren't accounting for the extra invalid
resource.

Simon's patch makes it easy to catch that off-by-one error (and fixes the
problem with the comma).

P.s.: I still think that at some point we should fix PR37068 and have a
single string table.

[gchatelet]

Thx Andrea,

I'm a bit confused by the extra comma in the counter's name

"uops_dispatched_port:port_7,",  //HWPort7
                            ^

I don't think it should be here.

[gchatelet]

I'm a bit confused by the extra comma in the counter's name

This is actually fixed by this patch :) Thx!

utils/TableGen/SubtargetEmitter.cpp

689 ↗

(On Diff #142993)

Do you mind adding a line feed.

719 ↗

(On Diff #142993)

Thx for adding the space after the comment :)

[gbedwell]

What's the more general plan for regression testing exegesis? It looks like the issue with pfm failing to match counters was a regression introduced by rL329675 due to the trailing commas. It would be nice to be able to have regression tests for this sort of thing, especially as it's under active development so the churn is likely to be quite high along with the risk of introducing regressions. I know it gets complicated due to the libpfm dependency and the fact it's CPU specific, but it should still be a solvable problem.

[RKSimon]

In D45787#1071927, @gbedwell wrote:

What's the more general plan for regression testing exegesis? It looks like the issue with pfm failing to match counters was a regression introduced by rL329675 due to the trailing commas. It would be nice to be able to have regression tests for this sort of thing, especially as it's under active development so the churn is likely to be quite high along with the risk of introducing regressions. I know it gets complicated due to the libpfm dependency and the fact it's CPU specific, but it should still be a solvable problem.

I've raised PR37176 to track this - it took us the best part of a day to track down the trailing comma problem which was time that could have been saved if CI would have caught it for us.

[gbedwell]

In D45787#1071943, @RKSimon wrote:

I've raised PR37176 to track this

Thanks Simon!

llvm/trunk/utils/TableGen/SubtargetEmitter.cpp

for (const CodeGenRegisterFile &RD : ProcModel.RegisterFiles) {
     unsigned NumCostEntries = RD.Costs.size();
     OS << NumCostEntries << ", " << CostTblIndex << "},\n";
     CostTblIndex += NumCostEntries;
   }
   OS << "};\n";
 
   return CostTblIndex;
 }
+
 static bool EmitPfmIssueCountersTable(const CodeGenProcModel &ProcModel,
                                       raw_ostream &OS) {
-  std::vector<const Record *> CounterDefs(ProcModel.ProcResourceDefs.size());
+  unsigned NumCounterDefs = 1 + ProcModel.ProcResourceDefs.size();
+  std::vector<const Record *> CounterDefs(NumCounterDefs);
   bool HasCounters = false;
   for (const Record *CounterDef : ProcModel.PfmIssueCounterDefs) {
     const Record *&CD = CounterDefs[ProcModel.getProcResourceIdx(
         CounterDef->getValueAsDef("Resource"))];
     if (CD) {
       PrintFatalError(CounterDef->getLoc(),
                       "multiple issue counters for " +
                           CounterDef->getValueAsDef("Resource")->getName());
     }
     CD = CounterDef;
     HasCounters = true;
   }
   if (!HasCounters) {
     return false;
   }
   OS << "\nstatic const char* " << ProcModel.ModelName
      << "PfmIssueCounters[] = {\n";
-  for (const Record *CounterDef : CounterDefs) {
+  for (unsigned i = 0; i != NumCounterDefs; ++i) {
+    const Record *CounterDef = CounterDefs[i];
     if (CounterDef) {
       const auto PfmCounters = CounterDef->getValueAsListOfStrings("Counters");
       if (PfmCounters.empty())
         PrintFatalError(CounterDef->getLoc(), "empty counter list");
-      for (const StringRef CounterName : PfmCounters)
-        OS << "  \"" << CounterName << ",\"";
-      OS << ",  //" << CounterDef->getValueAsDef("Resource")->getName() << "\n";
+      OS << "  \"" << PfmCounters[0];
+      for (unsigned p = 1, e = PfmCounters.size(); p != e; ++p)
+        OS << ",\" \"" << PfmCounters[p];
+      OS << "\",  // #" << i << " = ";
+      OS << CounterDef->getValueAsDef("Resource")->getName() << "\n";
     } else {
-      OS << "  nullptr,\n";
+      OS << "  nullptr, // #" << i << "\n";
     }
   }
   OS << "};\n";
   return true;
 }
 
 static void EmitPfmCounters(const CodeGenProcModel &ProcModel,
                             const bool HasPfmIssueCounters, raw_ostream &OS) {