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

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

Merged
merged 1 commit into from Jun 14, 2022

Conversation

tamiko
Copy link
Member

@tamiko tamiko commented Jun 13, 2022

We use Tpetra templates with types::global_dof_index throughout our
codebase - so we should also check for compatibility with said index
type.

Alternatively, this information is also available in
TpetraCore_config.h:

  /* #undef HAVE_TPETRA_INST_INT_INT */
  /* #undef HAVE_TPETRA_INST_INT_LONG */
  #define HAVE_TPETRA_INST_INT_LONG_LONG
  /* #undef HAVE_TPETRA_INST_INT_UNSIGNED */
  /* #undef HAVE_TPETRA_INST_INT_UNSIGNED_LONG */

In reference to #13972

#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.

@tamiko
Copy link
Member Author

tamiko commented Jun 14, 2022

/rebuild

We use Tpetra templates with types::global_dof_index throughout our
codebase - so we should also check for compatibility with said index
type.

Alternatively, this information is also available in
TpetraCore_config.h:

  /* #undef HAVE_TPETRA_INST_INT_INT */
  /* #undef HAVE_TPETRA_INST_INT_LONG */
  #define HAVE_TPETRA_INST_INT_LONG_LONG
  /* #undef HAVE_TPETRA_INST_INT_UNSIGNED */
  /* #undef HAVE_TPETRA_INST_INT_UNSIGNED_LONG */
@tamiko tamiko merged commit a42bb0d into dealii:master Jun 14, 2022
mkghadban pushed a commit to OpenFCST/dealii that referenced this pull request Sep 8, 2022
CMake: Use deal.II's global index type in Tpetra compatibility test
@tamiko tamiko deleted the fix_trilinos_checks_02 branch November 30, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants