search

hypre-space / hypre

Parallel solvers for sparse linear systems featuring multigrid methods.
https://www.llnl.gov/casc/hypre/
Other
651 stars 182 forks source link

Potentially incorrect `hypre_assert` #1096

Open v-dobrev opened 1 month ago

v-dobrev commented 1 month ago

When running MFEM tests with HYPRE built with --enable-debug and no GPU support the following check fails in a few MFEM examples: https://github.com/hypre-space/hypre/blob/0dcae3ec7c069785ea25d25aa0bc0c7aa8b0be8d/src/parcsr_ls/ame.c#L464

In these failures, the memory location on the left is HYPRE_MEMORY_DEVICE -- this comes from the standard hypre contructor, e.g. here: https://github.com/hypre-space/hypre/blob/0dcae3ec7c069785ea25d25aa0bc0c7aa8b0be8d/src/seq_mv/csr_matrix.c#L42 Note that even when hypre is configured without GPU support, hypre_HandleMemoryLocation(hypre_handle()) returns HYPRE_MEMORY_DEVICE by default, as seen here: https://github.com/hypre-space/hypre/blob/0dcae3ec7c069785ea25d25aa0bc0c7aa8b0be8d/src/utilities/general.c#L35

On the other hand, the memory location in the right hand side of the hypre_assert in HYPRE_MEMORY_HOST which comes from the memory location of ams_data->A: https://github.com/hypre-space/hypre/blob/0dcae3ec7c069785ea25d25aa0bc0c7aa8b0be8d/src/parcsr_ls/ame.c#L366 which, in turn, is set by MFEM to HYPRE_MEMORY_HOST since the matrix is on the host.

What would be the best way to resolve this?

victorapm commented 1 month ago

@liruipeng, should we update the assertion to test for the actual memory location instead?

liruipeng commented 1 month ago

Hi @v-dobrev Thank you for reporting this issue. This is a code design philosophy we have had from the beginning of the GPU work. When hypre is not configured with GPU, we treat HOST to be the "DEVICE", so there is no #ifdef of setting the default memory space to HYPRE_MEMORY_DEVICE, regardless of GPU or not. And the assert (among many others in the same form I guess) basically just say A and G lives in the same "device" space. Apparently, MFEM treat this differently. Does it work in the GPU case?

v-dobrev commented 1 month ago

When hypre is built for GPU, there's no issue -- MFEM sets the memory location to HYPRE_MEMORY_DEVICE.

I guees the question is: do you consider it an error if a user provides to hypre_AMESetup (or other similar functions) a matrix that has memory location HYPRE_MEMORY_HOST when hypre is built without GPU support? If you consider it a user error, we can close this issue, and we can make adjustments in MFEM to address the problem on our side. If you don't consider it a user error, then you probably will want to change the hypre_assert in some way to allow the case I ran into.