This is an archive of the discontinued LLVM Phabricator instance.

[llvm-exegesis] Fix PfmIssueCountersTable creation
ClosedPublic

Authored by RKSimon on Apr 18 2018, 2:00 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Apr 18 2018, 2:00 PM

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?

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.

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 accepted this revision.Apr 19 2018, 2:39 AM

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 :)

This revision is now accepted and ready to land.Apr 19 2018, 2:39 AM

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.

This revision was automatically updated to reflect the committed changes.

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.

I've raised PR37176 to track this

Thanks Simon!