Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance analysis #76

Open
wants to merge 9 commits into
base: devel
Choose a base branch
from
Open

Performance analysis #76

wants to merge 9 commits into from

Conversation

chrispbradley
Copy link
Member

Changes to the CMake scripts to allow for performance analysis.

elseif(OC_INSTRUMENTATION STREQUAL "none")
#Do nothing
else()
#error?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should at least have a message output here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a WARNING here. We can upgrade this to a FATAL_ERROR if you think that is appropriate.

@@ -340,26 +413,33 @@ if (OC_USE_IRON OR OC_DEPENDENCIES_ONLY)
find_package(SLEPC ${SLEPC_VERSION} QUIET)
if(NOT SLEPC_FOUND)
SET(SLEPC_FWD_DEPS IRON)
foreach(dependency IN_LIST HYPRE;MUMPS;PARMETIS;PASTIX;PTSCOTCH;SCALAPACK;SUITESPARSE;SUPERLU;SUPERLU_DIST;SUNDIALS)
if(PETSC_WITH_${dependency} AND OC_USE_${dependency})
Copy link
Contributor

@hsorby hsorby Nov 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not SLEPC specific?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SLEPc works through PETSc. You can't have it without having PETSc as well. Hence the reference to PETSc for the SLEPc dependencies.

endif()
endif()

message(STATUS "DEBUG: OC_INSTRUMENTATION = ${OC_INSTRUMENTATION}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are debug messages they should by default be hidden.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've commented these out. However, what we should have is a better solution to debugging CMake. I would advocate having a CMakeScript (OCUtilities.cmake or OCDebug.cmake or something) that doesn't rely on to many OC variables so that it can be included at the top of all of the CMake scripts that we have. We could then have something like in this file

function (ocdebug message)
if(DEFINED OC_DEBUG_CMAKE)
message(STATUS "DEBUG: ${message}"
endif()
endfunction()

Then, in all the CMake scripts we would have something like

include(OCDebug.cmake)

and

ocdebug("Debug output information")

This would unify all the messaged and messagev functions scattered throughout the scripts. We could then leave them uncommented as they wouldn't produce output unless somebody did a

cmake -DOC_DEBUG_CMAKE=1

or something.


# last ditch try -- if nothing works so far, just try running the regular compiler and
# see if we can create an MPI executable.
set(regular_compiler_worked 0)
if (NOT MPI_${lang}_LIBRARIES OR NOT MPI_${lang}_INCLUDE_PATH)
if (NOT MPI_${lang}_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping the full implications of this change are understood.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are trying to find an MPI compiler. This is done via an interrogate_mpi_compiler call which sets the MPI_${lang}_FOUND variable based directly on the LIBRARIES and INCLUDE_PATH variables as above.

@@ -132,7 +132,7 @@ include(FindPackageHandleStandardArgs)
# Comment the message command line to shut up the script
macro(messagev TEXT)
if (NOT WIN32) #The backslashes in the path elements break cmake :-(
#message(STATUS "FindMPI: ${TEXT}")
#message(STATUS "FindMPId: ${TEXT}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the d?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fat fingers? Removed.

@@ -231,7 +249,8 @@ else()
# Allow all paths, and add an extra path if set
set(PATHOPT )
# Start with MPI_HOME from the environment, of given
set(_MPI_PREFIX_PATH $ENV{MPI_HOME})
#set(_MPI_PREFIX_PATH $ENV{MPI_HOME})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a comment as to why this is not desired?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment. Basically some MPI environs set this when they are installed and this can then clash with the MPI environ specified via -DMPI=...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants