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

Potentially incorrect hypre_assert #1096

Open
v-dobrev opened this issue Jun 3, 2024 · 3 comments
Open

Potentially incorrect hypre_assert #1096

v-dobrev opened this issue Jun 3, 2024 · 3 comments

Comments

@v-dobrev
Copy link

v-dobrev commented Jun 3, 2024

When running MFEM tests with HYPRE built with --enable-debug and no GPU support the following check fails in a few MFEM examples:

hypre_assert( hypre_ParCSRMatrixMemoryLocation(ams_data->G) == memory_location);

In these failures, the memory location on the left is HYPRE_MEMORY_DEVICE -- this comes from the standard hypre contructor, e.g. here:

hypre_CSRMatrixMemoryLocation(matrix) = hypre_HandleMemoryLocation(hypre_handle());

Note that even when hypre is configured without GPU support, hypre_HandleMemoryLocation(hypre_handle()) returns HYPRE_MEMORY_DEVICE by default, as seen here:
hypre_HandleMemoryLocation(hypre_handle_) = HYPRE_MEMORY_DEVICE;

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:

HYPRE_MemoryLocation memory_location = hypre_ParCSRMatrixMemoryLocation(ams_data -> A);

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
Copy link
Contributor

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

@liruipeng
Copy link
Contributor

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
Copy link
Author

v-dobrev commented Jun 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants