[llvm-exegesis] Unset HAVE_LIBPFM if the kernel is too old.

[oontvoo]

Summary

Follow up to breakages reported in D77422

[mgorny]

You shouldn't be checking the running kernel but the version of the kernel headers. Think of build hosts building LLVM for an entirely different system altogether.

[lebedev.ri]

In D84076#2159963, @mgorny wrote:

You shouldn't be checking the running kernel but the version of the kernel headers. Think of build hosts building LLVM for an entirely different system altogether.

To add to that, you shouldn't hardcode version, but instead check that the problematic field is present or not.
And, i'm not sure this is a good idea. If the kernel is too old, i'd suggest only disabling the functionality
that wants that new field, not the entire libpfm

[oontvoo]

Use Cmake's CheckStructHasMember to check for cycles field.

[oontvoo]

In D84076#2160082, @lebedev.ri wrote:

In D84076#2159963, @mgorny wrote:

You shouldn't be checking the running kernel but the version of the kernel headers. Think of build hosts building LLVM for an entirely different system altogether.

To add to that, you shouldn't hardcode version, but instead check that the problematic field is present or not.
And, i'm not sure this is a good idea. If the kernel is too old, i'd suggest only disabling the functionality
that wants that new field, not the entire libpfm

Good point! I've updated the cmake file to look for the field and set a new flag accordingly
Thanks!

[oontvoo]

typos

[mgorny]

Did you forget to include a patch that uses LIBPFM_HAS_FIELD_CYCLES?

[oontvoo]

In D84076#2168614, @mgorny wrote:

Did you forget to include a patch that uses LIBPFM_HAS_FIELD_CYCLES?

No, actually. The patch that uses it is D77422
I was planning on letting this one go in first, then rebase the other one.

[oontvoo]

CheckStructHasMember didn't work (because cycles is a bit field) so updated it to just compile a small program that accesses .cycles.

[ondrasej]

llvm/cmake/modules/FindLibpfm.cmake

20

the field 'cycles'.

21

I'd finish the sentence with something like '...because 'cycles' is a bit field which is not supported by CheckStructHasMember.'

23

In https://reviews.llvm.org/D77422, we're including perfmon/perf_event.h, we should use the same header in both places.

29

Nit: Consider indenting the C++ source (two spaces more than CHECK_CXX_SOURCE_COMPILES), or moving it into a variable so that it doesn't break the indentation of the CMake code.

llvm/include/llvm/Config/config.h.cmake

100 ↗

(On Diff #280262)

1. Remove one space between 'struct' and 'has'.
2. Add a period at the end of the sentence.
3. ...the 'perf_branch_entry' struct... (to be consistent with using apostrophes for names in the rest of this file).

[oontvoo]

Updated diff

[ondrasej]

llvm/cmake/modules/FindLibpfm.cmake

20

Nit: cycles (the field name is in plural)

[oontvoo]

'cycle' -> 'cycles'

[mgorny]

I'll remove my earlier protest but let's give others a chance to review.

llvm/cmake/modules/FindLibpfm.cmake

 # CMake module for finding libpfm4.
 #
 # If successful, the following variables will be defined:
 # HAVE_LIBPFM
 #
 # Libpfm can be disabled by setting LLVM_ENABLE_LIBPFM to 0.
 
 include(CheckIncludeFile)
 include(CheckLibraryExists)
+include(CheckStructHasMember)
 
 if (LLVM_ENABLE_LIBPFM)
   check_library_exists(pfm pfm_initialize "" HAVE_LIBPFM_INITIALIZE)
   if(HAVE_LIBPFM_INITIALIZE)
     check_include_file(perfmon/perf_event.h HAVE_PERFMON_PERF_EVENT_H)
     check_include_file(perfmon/pfmlib.h HAVE_PERFMON_PFMLIB_H)
     check_include_file(perfmon/pfmlib_perf_event.h HAVE_PERFMON_PFMLIB_PERF_EVENT_H)
     if(HAVE_PERFMON_PERF_EVENT_H AND HAVE_PERFMON_PFMLIB_H AND HAVE_PERFMON_PFMLIB_PERF_EVENT_H)
       set(HAVE_LIBPFM 1)
+      # Check to see if this version has `cycles` in `struct perf_branch_entry`
+      Check_struct_has_member("struct perf_branch_entry" cycles perfmon/perf_event.h HAS_CYCLES LANGUAGE C)
+      if(HAS_CYCLES)
+        set(LIBPFM_HAS_FIELD_CYCLES 1)
+      endif()
     endif()
   endif()
 endif()