pfm: Use a more standard CMake approach for finding libpfm #1628

chipot · 2023-07-10T16:09:28Z

This PR is trying to address #1627

I reworked FindPFM.cmake to use find_library and find_path instead of check_library_exists and check_include_file. The main problem with these two is that they expect CFLAGS and LDFLAGS to be set correctly before the call.

The module now creates an IMPORTED target PFM::libpfm and we can link to it as expected.

google-cla · 2023-07-10T16:09:34Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

dmah42 · 2023-07-10T16:34:30Z

fantastic! i wonder if this is why the CI bots fail the pfm tests... cc @macandy13

dmah42 · 2023-07-10T16:44:28Z

CI failures are unrelated

dmah42 · 2023-07-10T16:46:56Z

thanks!

LebedevRI · 2023-07-10T16:50:37Z

cmake/Modules/FindPFM.cmake

-set_package_properties(PFM PROPERTIES
-                       URL http://perfmon2.sourceforge.net/
-                       DESCRIPTION "a helper library to develop monitoring tools"
-                       PURPOSE "Used to program specific performance monitoring events")


These should remain

added back with a followup commit

LebedevRI · 2023-07-10T16:51:44Z

AUTHORS

 Eric Fiselier <[email protected]>
 Eugene Zhuk <[email protected]>
 Evgeny Safronov <[email protected]>
+Fabien Pichot <[email protected]>


I know almost-sorted files is not fun to deal with,
but would it please be possible to not re-sort it here,
but just add new entries to the right-ish place?

LebedevRI · 2023-07-10T16:55:29Z

cmake/Modules/FindPFM.cmake

+if (PFM_FOUND AND NOT TARGET PFM::libpfm)
+    add_library(PFM::libpfm UNKNOWN IMPORTED)
+    set_target_properties(PFM::libpfm PROPERTIES


Is there a convention on the names of imported targets?
If not, i think PFM::PFM might be better-ish?

LebedevRI

Yes please, this is nice.

macandy13 · 2023-07-11T08:54:07Z

fantastic! i wonder if this is why the CI bots fail the pfm tests... cc @macandy13

Hm, I don't see test failures related to pfm. I also think the pfm tests are never actually run on the CI machines, because they do not support performance counters at all (which is what I tried in one of the recent prs but sadly had to remove from the pr again).

Am I missing something?

dmah42 · 2023-07-11T08:58:33Z

i was suggesting that the current version of this code that doesn't do a great job of finding pfm might be the reason why the CI machines can't run the tests. maybe with this change they can?

macandy13 · 2023-07-11T09:04:58Z

When I tried the tests, the problem was not that libpfm wasn't found (I was using bazel for doing this), it was that the underlying VM does not report any performance counters at all. The pfm tests actually expect that the system at least reports a few standard ones.

Do you have a link to the error you were seeing?

dmah42 · 2023-07-11T09:05:54Z

i didn't check any errors, i was just being optimistic.

macandy13 · 2023-07-11T09:10:53Z

Ok. For the record, this is the failure I was seeing when trying to enable pfm CI tests: https://github.com/google/benchmark/actions/runs/5484545652/jobs/9992228532#step:5:2830

[ RUN      ] PerfCountersTest.OneCounter
***WARNING** Failed to get a file descriptor for performance counter CYCLES. Ignoring
test/perf_counters_gtest.cc:36: Failure
Expected equality of these values:
  PerfCounters::Create({kGenericPerfEvent1}).num_counters()
    Which is: 0
  1
[  FAILED  ] PerfCountersTest.OneCounter (0 ms)

chipot force-pushed the find_pfm branch from a3c643b to 0c1e03c Compare July 10, 2023 16:10

pfm: Use a more standard CMake approach for finding libpfm

c3b2978

chipot force-pushed the find_pfm branch from 0c1e03c to c3b2978 Compare July 10, 2023 16:12

add myself and sort AUTHORS & CONTRIBUTORS

6e87443

dmah42 approved these changes Jul 10, 2023

View reviewed changes

dmah42 merged commit 8805bd0 into google:main Jul 10, 2023

LebedevRI reviewed Jul 10, 2023

View reviewed changes

chipot deleted the find_pfm branch July 11, 2023 10:14

pfm: Use a more standard CMake approach for finding libpfm #1628

pfm: Use a more standard CMake approach for finding libpfm #1628

Uh oh!

Conversation

chipot commented Jul 10, 2023

Uh oh!

google-cla bot commented Jul 10, 2023

Uh oh!

dmah42 commented Jul 10, 2023

Uh oh!

dmah42 commented Jul 10, 2023

Uh oh!

dmah42 commented Jul 10, 2023

Uh oh!

LebedevRI Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

dmah42 Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

LebedevRI Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

LebedevRI Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

LebedevRI left a comment

Choose a reason for hiding this comment

Uh oh!

macandy13 commented Jul 11, 2023

Uh oh!

dmah42 commented Jul 11, 2023

Uh oh!

macandy13 commented Jul 11, 2023

Uh oh!

dmah42 commented Jul 11, 2023

Uh oh!

macandy13 commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants