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

CMake: Use deal.II's global index type in Tpetra compatibility test #13973

Merged
merged 1 commit into from
Jun 14, 2022
Merged

CMake: Use deal.II's global index type in Tpetra compatibility test #13973

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions cmake/configure/configure_20_trilinos.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,14 +227,22 @@ MACRO(FEATURE_TRILINOS_FIND_EXTERNAL var)
# supply the correct compiler and linker flags:
ADD_FLAGS(CMAKE_REQUIRED_FLAGS "${DEAL_II_CXX_FLAGS} ${DEAL_II_LINKER_FLAGS}")

IF(DEAL_II_WITH_64BIT_INDICES)
SET(_global_index_type "std::uint64_t")
ELSE()
SET(_global_index_type "unsigned int")
ENDIF()

CHECK_CXX_SOURCE_COMPILES(
"
#include <cstdint>
#include <Tpetra_Vector.hpp>
int
main()
{
using LO = int;
using GO = unsigned int;
using GO = ${_global_index_type};
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the only valid Tpetra types seem to be int (aka int32_t) and long long (aka int64_t).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure I undestand. Ar you saying using an unsigned type here doesn't work?

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, using an unsigned type here doesn't work because Tpetra switched to int (aka signed int32_t) or long (aka signed int64_t) as global ordinal (the latter is the default). According to the Tpetra documentation using an unsigned integer is strongly discouraged. Trying it out anyway will lead to (a) either the build system telling you during configure that you cannot use an unsigned integer, and if you work around that, (b) a compile time error with a static assertion in Kokkos.

So the bottom line is that we cannot use Tpetra with using GO = unsigned int from Trilinos version 13.2 onwards. We will have to create a compatibility layer to cast between signed and unsigned integer type and make sure to always use the signed int32_t or int64_t types instead of uint32_t, or uint64_t that we use internally.

So bottom line is that this check will always fail for Trilinos 13.2 onwards.

Copy link
Member

Choose a reason for hiding this comment

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

For me, 13.2 works fine with GO = unsigned int. I get a warning a configure time but it compiles fine.

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 tried for about 10 hours to beat the Gentoo package [1] into shape with no success:

Trilinos-trilinos-release-13-4-0/packages/kokkos-kernels/src/common/KokkosKernels_Handle.hpp:64:41: error: static assertion failed: KokkosKernelsHandle requires that lno_t_ (ordinal) is a signed integer type.
   64 |   static_assert(std::is_signed<lno_t_>::value,
      |                                         ^~~~~
Trilinos-trilinos-release-13-4-0/packages/kokkos-kernels/src/sparse/KokkosSparse_CrsMatrix.hpp:377:36: error: static assertion failed: CrsMatrix requires that OrdinalType is a signed integer type.
  377 |       std::is_signed<OrdinalType>::value,
      |

This was a compilation with the following modification:

diff --git a/sci-libs/trilinos/trilinos-13.4.0-r3.ebuild b/sci-libs/trilinos/trilinos-13.4.0-r3.ebuild
index 9996b79bd35..563c04b4d80 100644
--- a/sci-libs/trilinos/trilinos-13.4.0-r3.ebuild
+++ b/sci-libs/trilinos/trilinos-13.4.0-r3.ebuild
@@ -112,6 +112,7 @@ src_configure() {
                -DTrilinos_INSTALL_INCLUDE_DIR="${EPREFIX}/usr/include/trilinos"
                -DTrilinos_INSTALL_LIB_DIR="${EPREFIX}/usr/$(get_libdir)/trilinos"
                -DTrilinos_ENABLE_ALL_PACKAGES="$(usex all-packages)"
+               -DTrilinos_ENABLE_EXPLICIT_INSTANTIATION=ON
                -DTrilinos_ENABLE_Adelus=OFF
                -DTrilinos_ENABLE_Moertel=OFF
                -DTrilinos_ENABLE_PyTrilinos=OFF
@@ -183,6 +184,7 @@ src_configure() {
                -DAmesos2_ENABLE_LAPACK=ON
                -DAmesos2_ENABLE_MUMPS=OFF
                -DTpetra_INST_SERIAL=ON
+               -DTpetra_INST_INT_UNSIGNED=ON
        )

But the real issue here is that this isn't really a good solution. For two reasons:

  • First, I cannot deploy GO set to unsigned int with good conscience for Debian and Ubuntu. This would be highly surprising for users and contrary to the intention of the upstream Trilinos authors.
  • Second, the Trilinos upstream authors are pretty clear about where this is going:
CMake Warning at packages/tpetra/CMakeLists.txt:747 (MESSAGE):
  Tpetra: You set Tpetra_INST_INT_UNSIGNED.  This means that you want to use
  GlobalOrdinal = unsigned with Tpetra objects.  Please don't do that.  I
  only maintain this option for backwards compatibility.  While I have gone
  through some effort to make unsigned GlobalOrdinal types work in Tpetra,
  they are not tested and thus I don't want to promise that they still work.

So it looks like that the only meaningful course of action at some point will be to simply support the default int64_t ordinal that Tpetra is using - for example by making 64bits the default in deal.II and providing a good translation between uint64_t and int64_t when needed.

[1] https://github.com/gentoo/gentoo/blob/master/sci-libs/trilinos/trilinos-13.4.0-r3.ebuild

Copy link
Member

Choose a reason for hiding this comment

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

Several packages depends on KokkosKernels so it might not be Tpetra itself that creates the problem but another package. I agree that we need to get away from using unsigned integers. It will not be supported in the future anyway.

Copy link
Member

Choose a reason for hiding this comment

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

The question is, do we need (or want) to do something for the release now?

Copy link
Member

Choose a reason for hiding this comment

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

In terms of checking all places where we use Tpetra?

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 think we will have to fix this after the release.

In particular, I tried to compile deal.II with a signed int64_t yesterday. This wasn't particularly successful and there is a lot of work to clean this up.

So what about we actually merge this pull request that will ensure that Tpetra is available with a compatible type - even though it will likely imply that it doesn't get enabled for most users.

using map_type = Tpetra::Map<LO, GO>;
Teuchos::RCP<const map_type> dummy_map = Teuchos::rcp(new map_type());
Tpetra::Vector<double, LO, GO> dummy_vector(dummy_map);
Expand Down