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

Removed the preprocessor branch for KOKKOS_ENABLE_PROFILING #3115

Conversation

DavidPoliakoff
Copy link
Contributor

You know, the one that leads to eight follow-up pull requests every time I touch Kokkos?

Importantly, anywhere we were guarding dlfcn.h with KOKKOS_ENABLE_PROFILING, I changed that to KOKKOS_ENABLE_LIBDL. Once this passes or fails CI I'll give it another look, but it would be nice if afterwards another careful developer would give it a once-over, make sure there's nothing subtle I missed

@DavidPoliakoff
Copy link
Contributor Author

Addresses #3095

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Would you mind also replacing Kokkos_ENABLE_PROFILING with Kokkos_ENABLE_LIBDL in .jenkins?

I am currently getting

/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp: In function ‘void Kokkos::Tools::initialize()’:
/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp:314:54: error: ‘RTLD_NOW’ was not declared in this scope
     firstProfileLibrary = dlopen(profileLibraryName, RTLD_NOW | RTLD_GLOBAL);
                                                      ^~~~~~~~
/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp:314:65: error: ‘RTLD_GLOBAL’ was not declared in this scope
     firstProfileLibrary = dlopen(profileLibraryName, RTLD_NOW | RTLD_GLOBAL);
                                                                 ^~~~~~~~~~~
/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp:314:27: error: ‘dlopen’ was not declared in this scope
     firstProfileLibrary = dlopen(profileLibraryName, RTLD_NOW | RTLD_GLOBAL);
                           ^~~~~~
/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp:314:27: note: suggested alternative: ‘popen’
     firstProfileLibrary = dlopen(profileLibraryName, RTLD_NOW | RTLD_GLOBAL);
                           ^~~~~~
                           popen
/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp:320:64: error: ‘dlerror’ was not declared in this scope
                 << ", RTLD_NOW | RTLD_GLOBAL) failed with " << dlerror()
                                                                ^~~~~~~
/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp:320:64: note: suggested alternative: ‘perror’
                 << ", RTLD_NOW | RTLD_GLOBAL) failed with " << dlerror()
                                                                ^~~~~~~
                                                                perror
/tmp/kokkos_new/core/src/impl/Kokkos_Profiling.cpp:331:17: error: ‘dlsym’ was not declared in this scope
       auto p1 = dlsym(firstProfileLibrary, "kokkosp_begin_parallel_for");
                 ^~~~~

if I try to disable LIBDL. There seem to be a few guards missing in core/src/impl/Kokkos_Profiling.cpp. Otherwise, this looks good.

Of course, you also need to rebase.

@@ -6,7 +6,8 @@
#if !defined(KOKKOS_FOR_SIERRA)

#if !defined(KOKKOS_MACROS_HPP) || defined(KOKKOS_CORE_CONFIG_H)
#error "Don't include KokkosCore_config.h directly; include Kokkos_Macros.hpp instead."
#error \
"Don't include KokkosCore_config.h directly; include Kokkos_Macros.hpp instead."
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't run clang-format on this file before?

Copy link
Member

Choose a reason for hiding this comment

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

yeah the clang-format script does only apply to source files.

@DavidPoliakoff
Copy link
Contributor Author

@masterleinad : I think I've addressed your comments. I'll wait until it passes CI, but if it does this is ready for another review

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

You should move free(envProfileCopy); in line 492 to line 423.

core/unit_test/CMakeLists.txt Show resolved Hide resolved
core/src/impl/Kokkos_Profiling.cpp Outdated Show resolved Hide resolved
@DavidPoliakoff
Copy link
Contributor Author

Good further catches, Daniel, thanks for the second pair of eyes on this

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

LGTM

@DavidPoliakoff
Copy link
Contributor Author

In my mind this is ready to merge, let me know if there are any additional comments. Looking forward to it, this is a huge maintenance debt gone

Kokkos::Profiling::initialize();
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Why were we initializing here in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was added by @crtrott here: fae0d00 , apparently it addressed this issue #192 , where Trilinos used to initialize execution spaces individually rather than initializing Kokkos. If we're going to change that, I'd prefer it be in another PR

@dalg24 dalg24 merged commit 30edca8 into kokkos:develop Jul 1, 2020
ndellingwood added a commit to ndellingwood/Trilinos that referenced this pull request Jul 30, 2020
Kokkos_ENABLE_PROFILING was removed as a configure option
and macro in Kokkos with pull request
kokkos/kokkos#3115
This change updates Teuchos' accordingly
ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 17, 2020
Kokkos_ENABLE_PROFILING was removed as a configure option
and macro in Kokkos with pull request
kokkos/kokkos#3115
This change updates Teuchos' accordingly
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.

None yet

4 participants