batched_lu_serial: fix for ambiguous ? operation error #512

ndellingwood · 2019-12-05T22:41:08Z

Detected during Trilinos integration testing.

ndellingwood · 2019-12-06T17:32:35Z

@kyungjoo-kim @brian-kelley or @srajama1 do any of you have a moment to review? Single line change for issue that came up during integration testing.

brian-kelley

Why do the types of tiny and -tiny not match? It's not just the lack of const on -tiny, since the fix also selects either tiny or mst(-tiny) (const vs. non-const).

mhoemmen · 2019-12-06T17:45:55Z

src/batched/KokkosBatched_LU_Serial_Internal.hpp

@@ -37,7 +37,8 @@ namespace KokkosBatched {
    const int k = (m < n ? m : n);
    if (k <= 0) return 0;

-    const auto       abs_tiny =  tiny > 0 ? tiny : -tiny;
+    using mst = typename MagnitudeScalarType<ValueType>::type;
+    const auto       abs_tiny =  tiny > 0 ? tiny : mst(-tiny);


Could the issue be that tiny has an unsigned integer type?

ndellingwood · 2019-12-06T17:51:33Z

@brian-kelley @mhoemmen the issue occurred with Sacado due to some nuances of expression templates, here's the sample error code snip:

/ascldap/users/ndellin/trilinos/Trilinos/kokkos-kernels/src/batched/KokkosBatched_LU_Serial_Internal.hpp(40): error: ambiguous "?" operation: second operand of type "const Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosCudaWrapperNode>" can be converted to third operand type "Sacado::MP::UnaryMinusOp<Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosCudaWrapperNode>>", and vice versa

I'm adding Eric @etphipp if more detailed explanation is needed.

brian-kelley · 2019-12-06T17:55:19Z

@ndellingwood OK, that makes sense. @etphipp Does Sacado provide specializations for Kokkos::ArithTraits? If it does, maybe we should use the ArithTraits<T>::abs(). Otherwise I'll approve.

ndellingwood · 2019-12-06T17:57:09Z

@ndellingwood OK, that makes sense. Does Sacado provide specializations for Kokkos::ArithTraits? If it does, maybe we should use the ArithTraits<T>::abs(). Otherwise I'll approve.

@etphipp does Sacado have specializations of ArithTraits so that would allow for use of abs instead of requiring type conversion to create -tiny?

mhoemmen · 2019-12-06T18:01:49Z

@ndellingwood wrote:

... the issue occurred with Sacado due to some nuances of expression templates....

Ah, that has bitten me before.

ndellingwood · 2019-12-06T20:25:17Z

@brian-kelley is it okay if we go ahead with this PR, and later I can submit new PR to replace with ArithTraits if it turns out there are ArithTraits specializations to support the failing case?

ndellingwood · 2019-12-06T20:27:48Z

Thanks @brian-kelley !

brian-kelley · 2019-12-06T20:27:58Z

@ndellingwood Of course, I mainly just wanted to understand what was happening and it makes sense.

etphipp · 2019-12-07T16:00:24Z

Yes there is an ArithTraits specialization for this scalar type.

Note that I was thinking yesterday that there might be a way for me fix these kinds of problems without having to put in the explicit conversion for -tiny, if I mark the unary expression template constructor explicit. I will experiment with that next week.

kyungjoo-kim · 2019-12-09T17:00:02Z

@ndellingwood Would you please tell me where we see this error ? So far the function is used in ifpack2 block tridiag and I isolate sacado instanciation to avoid ETI with the type. Thus, this means that the code is used in other packages.

ndellingwood · 2019-12-09T17:03:53Z

@kyungjoo-kim here is a more verbose copy of the error message that occurred during Trilinos integration testing:

/ascldap/users/ndellin/IntegrationTests-Blake/serial/trilinos-update/packages/kokkos-kernels/src/batched/KokkosBatched_LU_Serial_Internal.hpp(40): error: ambiguous "?" operation: second operand of type "const Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosSerialWrapperNode>" can be converted to third operand type
          "Sacado::MP::UnaryMinusOp<Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosSerialWrapperNode>>", and vice versa
      const auto       abs_tiny =  tiny > 0 ? tiny : -tiny;
                                                   ^
          detected during:
            instantiation of "int KokkosBatched::SerialLU_Internal<KokkosBatched::Algo::Level3::Unblocked>::invoke(int, int, ValueType *, int, int, KokkosBatched::MagnitudeScalarType<ValueType>::type) [with ValueType=Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosSerialWrapperNode>]" at line 70 of
                      "/ascldap/users/ndellin/IntegrationTests-Blake/serial/trilinos-update/packages/kokkos-kernels/src/batched/KokkosBatched_LU_Serial_Impl.hpp"
            instantiation of "int KokkosBatched::SerialLU<KokkosBatched::Algo::Level3::Unblocked>::invoke(const AViewType &, KokkosBatched::MagnitudeScalarType<AViewType::non_const_value_type>::type) [with AViewType=Kokkos::View<Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosSerialWrapperNode> **, Kokkos::LayoutLeft, Kokkos::Device<Kokkos::HostSpace::execution_space, Kokkos::HostSpace::memory_space>,
                      Kokkos::MemoryManaged>]" at line 96 of "/ascldap/users/ndellin/IntegrationTests-Blake/serial/trilinos-update/packages/tpetra/core/src/Tpetra_Details_scaleBlockDiagonal.hpp"
            instantiation of "void Tpetra::Details::inverseScaleBlockDiagonal(const MultiVectorType &, bool, MultiVectorType &) [with MultiVectorType=Tpetra::MultiVector<Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosSerialWrapperNode>, default_local_ordinal_type={Tpetra::Details::DefaultTypes::local_ordinal_type={int}}, default_global_ordinal_type={Tpetra::Details::DefaultTypes::global_ordinal_type={int}},
                      Kokkos_Compat_KokkosSerialWrapperNode>]" at line 638 of "/ascldap/users/ndellin/IntegrationTests-Blake/serial/trilinos-update/packages/xpetra/sup/Utils/Xpetra_MatrixUtils.hpp"
            instantiation of "void Xpetra::MatrixUtils<Scalar, LocalOrdinal, GlobalOrdinal, Node>::inverseScaleBlockDiagonal(const Xpetra::MultiVector<Scalar, LocalOrdinal, GlobalOrdinal, Node> &, bool, Xpetra::MultiVector<Scalar, LocalOrdinal, GlobalOrdinal, Node> &) [with Scalar=Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosSerialWrapperNode>,
                      LocalOrdinal=default_local_ordinal_type={Tpetra::Details::DefaultTypes::local_ordinal_type={int}}, GlobalOrdinal=default_global_ordinal_type={Tpetra::Details::DefaultTypes::global_ordinal_type={int}}, Node=Kokkos_Compat_KokkosSerialWrapperNode]" at line 120 of "/ascldap/users/ndellin/IntegrationTests-Blake/serial/trilinos-update/packages/muelu/src/Interface/../Transfers/Smoothed-Aggregation/MueLu_ScaledNullspaceFactory_def.hpp"
            instantiation of "void MueLu::ScaledNullspaceFactory<Scalar, LocalOrdinal, GlobalOrdinal, Node>::Build(MueLu::ScaledNullspaceFactory<Scalar, LocalOrdinal, GlobalOrdinal, Node>::Level &) const [with Scalar=Sacado::MP::Vector<SFS_int_double_16_DFN_CPU_default_local_ordinal_type_default_global_ordinal_type_Kokkos_Compat_KokkosSerialWrapperNode>, LocalOrdinal=default_local_ordinal_type={Tpetra::Details::DefaultTypes::local_ordinal_type={int}},
                      GlobalOrdinal=default_global_ordinal_type={Tpetra::Details::DefaultTypes::global_ordinal_type={int}}, Node=Kokkos_Compat_KokkosSerialWrapperNode]" at line 61 of "/ascldap/users/ndellin/IntegrationTests-Blake/serial/build_updated/packages/stokhos/src/MueLu_ScaledNullspaceFactory_MP_Vector_16_Serial.cpp"

compilation aborted for /ascldap/users/ndellin/IntegrationTests-Blake/serial/build_updated/packages/stokhos/src/MueLu_ScaledNullspaceFactory_MP_Vector_16_Serial.cpp (code 2)
make[2]: *** [packages/stokhos/src/CMakeFiles/stokhos_muelu_mp_16_serial.dir/build.make:1833: packages/stokhos/src/CMakeFiles/stokhos_muelu_mp_16_serial.dir/MueLu_ScaledNullspaceFactory_MP_Vector_16_Serial.cpp.o] Error 2

kyungjoo-kim · 2019-12-09T18:22:53Z

Oh.... I see it. @etphipp Is it safe to naively use temporal sacado variables inside of a functor ? KokkosKernels might use temporal variables without special constructors, which might not be working with DFAD types (SFAD should be fine and this MP Vector is also fine). Do we have Trilinos level cmake option to detect compatibility of ETI types ?

etphipp · 2019-12-09T18:26:11Z

Oh.... I see it. @etphipp Is it safe to naively use temporal sacado variables inside of a functor ? KokkosKernels might use temporal variables without special constructors, which might not be working with DFAD types (SFAD should be fine and this MP Vector is also fine). Do we have Trilinos level cmake option to detect compatibility of ETI types ?

It is always safe to use a temporary with any Sacado/Stokhos scalar type. It would be pretty much impossible to write complex kernels without temporaries!

That being said, I am about to push a PR for Sacado/Stokhos that makes this particular change unnecessary (although there are still situations where an explicit conversion is needed because of limitations of the standard).

batched_lu_serial: fix for ambiguous ? operation error

7f80f21

Detected during Trilinos integration testing.

ndellingwood requested review from kyungjoo-kim, brian-kelley and srajama1 December 5, 2019 22:41

brian-kelley reviewed Dec 6, 2019

View reviewed changes

mhoemmen reviewed Dec 6, 2019

View reviewed changes

ndellingwood mentioned this pull request Dec 6, 2019

Post-CMake merge and 3.0 items #486

Closed

3 tasks

brian-kelley approved these changes Dec 6, 2019

View reviewed changes

ndellingwood merged commit 10ca383 into develop Dec 6, 2019

ndellingwood deleted the SerialLU_Internal-fix branch December 6, 2019 20:27

etphipp mentioned this pull request Dec 9, 2019

Sacado/Stokhos: Make unary expression constructors explicit. trilinos/Trilinos#6411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batched_lu_serial: fix for ambiguous ? operation error #512

batched_lu_serial: fix for ambiguous ? operation error #512

ndellingwood commented Dec 5, 2019

ndellingwood commented Dec 6, 2019

brian-kelley left a comment

mhoemmen Dec 6, 2019

ndellingwood commented Dec 6, 2019 •

edited

Loading

brian-kelley commented Dec 6, 2019 •

edited

Loading

ndellingwood commented Dec 6, 2019

mhoemmen commented Dec 6, 2019

ndellingwood commented Dec 6, 2019

ndellingwood commented Dec 6, 2019

brian-kelley commented Dec 6, 2019

etphipp commented Dec 7, 2019

kyungjoo-kim commented Dec 9, 2019

ndellingwood commented Dec 9, 2019

kyungjoo-kim commented Dec 9, 2019

etphipp commented Dec 9, 2019

batched_lu_serial: fix for ambiguous ? operation error #512

batched_lu_serial: fix for ambiguous ? operation error #512

Conversation

ndellingwood commented Dec 5, 2019

ndellingwood commented Dec 6, 2019

brian-kelley left a comment

Choose a reason for hiding this comment

mhoemmen Dec 6, 2019

Choose a reason for hiding this comment

ndellingwood commented Dec 6, 2019 • edited Loading

brian-kelley commented Dec 6, 2019 • edited Loading

ndellingwood commented Dec 6, 2019

mhoemmen commented Dec 6, 2019

ndellingwood commented Dec 6, 2019

ndellingwood commented Dec 6, 2019

brian-kelley commented Dec 6, 2019

etphipp commented Dec 7, 2019

kyungjoo-kim commented Dec 9, 2019

ndellingwood commented Dec 9, 2019

kyungjoo-kim commented Dec 9, 2019

etphipp commented Dec 9, 2019

ndellingwood commented Dec 6, 2019 •

edited

Loading

brian-kelley commented Dec 6, 2019 •

edited

Loading