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

[Ondrej Sykora via Phabricator via llvm-commits]

ondrasej added inline comments.


================
Comment at: llvm/cmake/modules/FindLibpfm.cmake:20
       set(HAVE_LIBPFM 1)
+      # Check to see if perf_branch_entry has field cycle.
+      # We couldn't use CheckStructHasMember here because 'cycles' is a bit field.
----------------
the field 'cycles'.


================
Comment at: llvm/cmake/modules/FindLibpfm.cmake:21
+      # Check to see if perf_branch_entry has field cycle.
+      # We couldn't use CheckStructHasMember here because 'cycles' is a bit field.
+      CHECK_CXX_SOURCE_COMPILES("
----------------
I'd finish the sentence with something like '...because 'cycles' is a bit field which is not supported by CheckStructHasMember.'


================
Comment at: llvm/cmake/modules/FindLibpfm.cmake:23
+      CHECK_CXX_SOURCE_COMPILES("
+#include <linux/perf_event.h>
+int main() {
----------------
In https://reviews.llvm.org/D77422, we're including perfmon/perf_event.h, we should use the same header in both places.


================
Comment at: llvm/cmake/modules/FindLibpfm.cmake:29
+}
+" COMPILE_WITH_CYCLES)
+      if(COMPILE_WITH_CYCLES)
----------------
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.


================
Comment at: llvm/include/llvm/Config/config.h.cmake:100
 
+/* Define to 1 if perf_branch_entry struct  has field cycles */
+#cmakedefine LIBPFM_HAS_FIELD_CYCLES ${LIBPFM_HAS_FIELD_CYCLES}
----------------
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84076/new/

https://reviews.llvm.org/D84076