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

Address the pesky 'MPI_Comm as void*' problem with the SUNDIALS interfaces #15113

Merged
merged 4 commits into from Apr 20, 2023

Conversation

bangerth
Copy link
Member

This patch block #15086, and so I would ask for expedited review.

See #15086 (review) Fundamentally, this patch works around the issue mentioned here: LLNL/sundials#275

@sebproell @stefanozampini FYI.

@bangerth
Copy link
Member Author

The correct comment is here: #15086 (review)

Assert(v->content != nullptr, ExcInternalError());
auto *pContent =
reinterpret_cast<NVectorContent<VectorType> *>(v->content);
return pContent->get_mpi_communicator();
}
Copy link
Member

Choose a reason for hiding this comment

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

Why did

      template <typename VectorType,
                std::enable_if_t<is_serial_vector<VectorType>::value, int>>
      void *get_communicator_as_void_ptr(N_Vector)
      {
        // required by SUNDIALS: MPI-unaware vectors should return the nullptr
        // as comm
        return nullptr;
      }

disappear? Is &MPI_COMM_SELF==nullptr?

edit: Clarified "this".

Copy link
Member Author

Choose a reason for hiding this comment

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

There were two specializations of the function, one for serial and one for parallel vectors (enabled or disabled via std::enable_if) because the former do not have a get_communicator() member function.

I've outsourced that decision to the get_mpi_communicator() free function above now that returns MPI_COMM_SELF for serial vectors. Are you saying that we should just return nullptr if a vector is serial, regardless of whether we configured for MPI or not?

Copy link
Member Author

@bangerth bangerth Apr 19, 2023

Choose a reason for hiding this comment

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

Maybe something like this in the current place?

diff --git a/include/deal.II/sundials/n_vector.templates.h b/include/deal.II/sundials/n_vector.templates.h
index e5e9db4bd2..e4a42d659a 100644
--- a/include/deal.II/sundials/n_vector.templates.h
+++ b/include/deal.II/sundials/n_vector.templates.h
@@ -619,8 +619,12 @@ namespace SUNDIALS
         (void)v;
         return nullptr;
 #  else
-        // We need to cast away const here, as SUNDIALS demands a pure `void *`.
-        return &(const_cast<MPI_Comm &>(get_communicator<VectorType>(v)));
+        if (is_serial_vector<VectorType>::value == false)
+          // We need to cast away const here, as SUNDIALS demands a pure `void
+          // *`.
+          return &(const_cast<MPI_Comm &>(get_communicator<VectorType>(v)));
+        else
+          return nullptr;
 #  endif
       }
 

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 convinced myself that that is the right way to go and pushed an updated commit.

@bangerth
Copy link
Member Author

This required a bit more work, but it works now at least locally.

Copy link
Member

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

Looks OK to me.

@tjhei tjhei merged commit 70d13b7 into dealii:master Apr 20, 2023
14 checks passed
@bangerth bangerth deleted the nvector branch April 20, 2023 15:58
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

3 participants