This is an archive of the discontinued LLVM Phabricator instance.

[llvm-exegesis] Add a flag to disable libpfm even if present.
ClosedPublic

Authored by courbet on Apr 9 2018, 4:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Apr 9 2018, 4:53 AM

I don't know the build system very well but this looks good to me.

Perhaps document LLVM_ENABLE_LIBPFM in docs/CMake.rst?

courbet updated this revision to Diff 141620.Apr 9 2018, 5:06 AM

document LLVM_ENABLE_LIBPFM in docs/CMake.rst

Good point, done.

lebedev.ri added inline comments.
cmake/config-ix.cmake
93 ↗(On Diff #141620)

One more piece is missing, LLVM_ENABLE_LIBPFM needs to be checked where linking to pfm happens.

courbet added inline comments.Apr 9 2018, 6:14 AM
cmake/config-ix.cmake
93 ↗(On Diff #141620)

Thanks Roman,

This is already done by checking on HAVE_LIBPFM, see tools/llvm-exegesis/CMakeLists.txt:

if(HAVE_LIBPFM)
  target_link_libraries(llvm-exegesis PRIVATE pfm)
endif()
lebedev.ri added inline comments.Apr 9 2018, 6:32 AM
cmake/config-ix.cmake
93 ↗(On Diff #141620)

Exactly, and that is insufficient.
It should be

if(LLVM_ENABLE_LIBPFM AND HAVE_LIBPFM)
  target_link_libraries(llvm-exegesis PRIVATE pfm)
endif()

I'm not sure if LLVM can be built as part of some parent CMake project, so *this* may be not-an-issue,
but will just give a bad example for future changes. The problem is, what will happen if one
defines HAVE_LIBPFM (e.g. by using it in some parent cmake project), but disables LLVM_ENABLE_LIBPFM?

Similar problem: https://github.com/google/googletest/pull/975

courbet updated this revision to Diff 141637.Apr 9 2018, 6:49 AM
courbet marked an inline comment as done.

Double check for LLVM_ENABLE_LIBPFM and HAVE_LIBPFM when linking.

cmake/config-ix.cmake
93 ↗(On Diff #141620)

I see, that makes sense, thanks for the explanation.

lebedev.ri accepted this revision.Apr 9 2018, 9:38 AM

I don't really know LLVM's buildsystem, but this looks about right to me.
Sidenote:

cmake/config-ix.cmake
94 ↗(On Diff #141637)

Sidenote: normally, this should be in a cmake/Modules/FindPFM.cmake, and this should be just

if (LLVM_ENABLE_LIBPFM)
  find_package(PFM REQUIRED)
endif()
This revision is now accepted and ready to land.Apr 9 2018, 9:38 AM
rudskoy removed a subscriber: rudskoy.
This revision was automatically updated to reflect the committed changes.
courbet added inline comments.Apr 11 2018, 12:42 AM
cmake/config-ix.cmake
94 ↗(On Diff #141637)

Good idea. I've split this off to a separate module in r329783.