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.

Sign up for GitHub

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

Jump to bottom

Teuchos: Automatically enabling Tecuhos_ENABLE_THREAD_SAFE if you have Kokkos THREADS or OPENMP for the host #11946

Merged
merged 4 commits into from
Jun 7, 2023
Merged

Teuchos: Automatically enabling Tecuhos_ENABLE_THREAD_SAFE if you have Kokkos THREADS or OPENMP for the host #11946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1338338
Teuchos: Automatically enabling Tecuhos_ENABLE_THREAD_SAFE if you hav…
csiefer2 Jun 6, 2023
aa11d5e
Teuchos: Fixing catch() issues with C++ language drift
csiefer2 Jun 6, 2023
80cc3c7
Teuchos: Fixing cmake logic
csiefer2 Jun 6, 2023
21e643d
Remove non-existant subdir kokkos-kernels/common/common (#11921, #11863)
bartlettroscoe Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions packages/kokkos-kernels/common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,3 @@ LIST(APPEND KK_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/common/src)
LIST(APPEND KK_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/common/src/impl)
LIST(APPEND KK_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/common/src/tpls)
LIST(APPEND KK_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/common/unit_test)

Copy link
Contributor

Choose a reason for hiding this comment

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

These kokkos-kernels changes should have had a matching PR put in to the kernels repo, @bartlettroscoe can you submit a PR with these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Matching patch to kokkos-kernels is up kokkos/kokkos-kernels#1854

# Adding unit-tests
KOKKOSKERNELS_INCLUDE_DIRECTORIES(${CMAKE_CURRENT_BINARY_DIR}/common)
KOKKOSKERNELS_INCLUDE_DIRECTORIES(REQUIRED_DURING_INSTALLATION_TESTING ${CMAKE_CURRENT_SOURCE_DIR}/common)
22 changes: 21 additions & 1 deletion packages/teuchos/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,29 @@ IF (DEFINED BUILD_SHARED_LIBS)
ENDIF ()
ENDIF ()


# As per the discussion in #11921, we explicitly enable thread safe RCPs,
# if Kokkos has a host thread-parallel backend
SET(Teuchos_ENABLE_THREAD_SAFE_Default ${Trilinos_ENABLE_THREAD_SAFE})
IF(DEFINED ${PROJECT_NAME}_ENABLE_Kokkos AND ${PROJECT_NAME}_ENABLE_Kokkos)
Copy link
Member

@bartlettroscoe bartlettroscoe Jun 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This can be simplified as:

IF (${PROJECT_NAME}_ENABLE_Kokkos)

If a variable is undefined, it evaluates to false. See here which states:

if(<variable>)
True if given a variable that is defined to a value that is not a false
constant. False otherwise, including if the variable is undefined.

IF( (DEFINED Kokkos_ENABLE_THREADS AND Kokkos_ENABLE_THREADS) OR
(DEFINED Kokkos_ENABLE_OPENMP AND Kokkos_ENABLE_OPENMP) )
Comment on lines +260 to +261
Copy link
Member

@bartlettroscoe bartlettroscoe Jun 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This can be simplified to:

IF( Kokkos_ENABLE_THREADS OR Kokkos_ENABLE_OPENMP)

If a variable is undefined, it evaluates to false.

IF (NOT ${PACKAGE_NAME}_ENABLE_THREAD_SAFE)
MESSAGE(WARNING "A Kokkos host thread-parallel backend is enabled, requiring Trilinos to enable thread-safe RCP classes. You can do this manually by specifying Teuchos_ENABLE_THREAD_SAFE=ON")
Copy link
Member

Choose a reason for hiding this comment

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

So what does it mean if Kokkos_ENABLE_THREADS=ON and Teuchos_ENABLE_THREAD_SAFE=ON but Trilinos_ENABLE_THREAD_SAFE=OFF? But it seems like only teuchos/CMakeLists.txt is looking at this variable so it currently does not matter (but that might confuse and frustrate if these are inconsistent).

SET(Teuchos_ENABLE_THREAD_SAFE_Default ON)
ENDIF()
ENDIF()
ENDIF()

TRIBITS_ADD_OPTION_AND_DEFINE(
${PACKAGE_NAME}_ENABLE_THREAD_SAFE
HAVE_TEUCHOS_THREAD_SAFE
"Enable thread safe code for RCP classes."
${Trilinos_ENABLE_THREAD_SAFE}
${Teuchos_ENABLE_THREAD_SAFE_Default}
)



ASSERT_DEFINED(${PACKAGE_NAME}_ENABLE_THREAD_SAFE)
ASSERT_DEFINED(${PACKAGE_NAME}Core_ENABLE_Pthread)
COMBINED_OPTION(
Expand All @@ -270,6 +286,10 @@ COMBINED_OPTION(
"Enable thread safe unit tests for ${PACKAGE_NAME} Memory Management classes."
)





TRIBITS_ADD_OPTION_AND_DEFINE(
${PACKAGE_NAME}_ENABLE_ABC
HAVE_TEUCHOS_ARRAY_BOUNDSCHECK
Expand Down
2 changes: 1 addition & 1 deletion packages/teuchos/core/test/MemoryManagement/ArrayView_MT_UnitTests_Decl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ static void share_arrayview_to_threads(ArrayView<int> shared_arrayview,
++cycle;
}
}
catch (DanglingReferenceError) {
catch (DanglingReferenceError&) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

// loop ends - we got the dangling reference
index_tracker.danglingReference = cycle;
}
Expand Down
6 changes: 3 additions & 3 deletions packages/teuchos/core/test/MemoryManagement/Array_MT_UnitTests_Decl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,15 +363,15 @@ static void do_read_operations_on_array(RCP<arrayType> shared_array,
}
}
}
catch (DanglingReferenceError) {
catch (DanglingReferenceError&) {
// If test throws a dangling reference error, record the cycle it occurred.
index_tracker.danglingReference = cycle;
}
catch (RangeError) {
catch (RangeError&) {
// If tests throws a range error, record the cycle it occurred.
index_tracker.outOfRangeError = cycle;
}
catch (std::out_of_range) {
catch (std::out_of_range&) {
// We could also get std::out_of_range
// Note that here were are counting Trilinos RangeError and STL
// std::out_of_range as all the same - they both happen since the bad
Expand Down
2 changes: 1 addition & 1 deletion packages/teuchos/core/test/MemoryManagement/Ptr_MT_UnitTests_Decl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ static void share_ptr_to_threads(Ptr<int> shared_ptr, int theTestValue,
++cycle;
}
}
catch(DanglingReferenceError) {
catch(DanglingReferenceError&) {
// we got the dangling error as expected
index_tracker.danglingReference = cycle;
}
Expand Down