[PATCH] D45360: [MC][TableGen] Add optional libpfm counter names for ProcResUnits.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Nice patch.

Would it be difficult to have a single string table for all the PfmIssueCounters defined by the scheduling models?

At the moment, your patch introduces a PfmIssueCounters table for every model:

  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,
  };
  <snip>
  
  static const char* SkylakeClientModelPfmIssueCounters[] = {
    nullptr,
    nullptr,
    nullptr,
    "uops_dispatched_port:port_0,",  //SKLPort0
    "uops_dispatched_port:port_1,",  //SKLPort1
    "uops_dispatched_port:port_2,",  //SKLPort2
    "uops_dispatched_port:port_3,",  //SKLPort3
    "uops_dispatched_port:port_4,",  //SKLPort4
    "uops_dispatched_port:port_5,",  //SKLPort5
    "uops_dispatched_port:port_6,",  //SKLPort6
    "uops_dispatched_port:port_7,",  //SKLPort7
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
    nullptr,
  };

However, I think this is suboptimal for two reasons:

1. the per-model table is not well compressed because there is one entry per each processor resource. That means, we consume entries even for resources that don't have a counter associated.
2. (as you can see from the code snippet above), most strings are actually duplicated in each table.

If possible, ideally we should have a single compressed string table accessible by all models:

  nullptr  // invalid entry
  "uops_dispatched_port:port_0,"
  "uops_dispatched_port:port_1,"
  "uops_dispatched_port:port_2,"
  "uops_dispatched_port:port_3,"
  "uops_dispatched_port:port_4,"
  "uops_dispatched_port:port_5,"
  "uops_dispatched_port:port_6,"
  "uops_dispatched_port:port_7,"

The compressed table comes with the downside that it requires an extra mapping between processor resource IDs and indices to the string table. That mapping could be stored somewhere in the "ExtraInfo" table.

I hope this makes sense.

Overall, the patch looks good. I am also okay if you want to commit this patch first, and then improve the design in a follow-up patch.

-Andrea


Repository:
  rL LLVM

https://reviews.llvm.org/D45360