Skip to content

Conversation

@matthiasdiener
Copy link
Member

@matthiasdiener matthiasdiener commented Jul 8, 2022

@matthiasdiener matthiasdiener self-assigned this Jul 8, 2022
@matthiasdiener
Copy link
Member Author

matthiasdiener commented Aug 8, 2022

This works for wave.py with the current pyopencl and loopy versions, but lazy pulse-mpi.py crashes with a segfault on pocl-pthreads: https://gist.github.com/matthiasdiener/1018812d74dbb14f3418569f5c8ca2ed

Edit: limit_arg_size_nbytes was set to 1024 for this experiment.

cc @inducer

Edit: inducer/arraycontext#186 seems to have fixed this.

@matthiasdiener
Copy link
Member Author

What do you think of the get_reasonable_memory_pool interface @inducer?

@matthiasdiener matthiasdiener marked this pull request as ready for review August 10, 2022 21:10
@matthiasdiener matthiasdiener requested a review from inducer August 10, 2022 21:10
@matthiasdiener
Copy link
Member Author

From my perspective, this is ready for review. This should work whether the other PRs (e.g. inducer/pyopencl#452) are merged or not.

@matthiasdiener matthiasdiener requested a review from MTCam August 11, 2022 15:07
Comment on lines 438 to 450
def get_reasonable_memory_pool(ctx, queue):
"""Return an SVM or buffer memory pool based on what the device supports."""
from pyopencl.characterize import has_coarse_grain_buffer_svm
import pyopencl.tools as cl_tools

if has_coarse_grain_buffer_svm(queue.device) and hasattr(cl_tools, "SVMPool"):
logger.info("Using SVM-based memory pool")
return cl_tools.SVMPool(cl_tools.SVMAllocator( # pylint: disable=no-member
ctx, alignment=0, queue=queue))
else:
from warnings import warn
warn("No SVM support, returning a CL buffer-based memory pool")
return cl_tools.MemoryPool(cl_tools.ImmediateAllocator(queue))
Copy link
Member

Choose a reason for hiding this comment

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

Why simutils and not pyopencl ?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought was that this function is very specific to our use case - other applications using pyopencl probably won't care much for this function since it is unlikely they would prefer an SVM pool over a buffer pool. I thought about putting this in grudge, but it also doesn't feel relevant there.

Copy link
Member

Choose a reason for hiding this comment

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

I defer to your judgement on this, i was mostly curious

Copy link
Member

@MTCam MTCam left a comment

Choose a reason for hiding this comment

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

lgtm! we just gotta get the downstream examples updated.

@matthiasdiener matthiasdiener changed the title SVM args Use SVM memory pool Aug 12, 2022
ctx, alignment=0, queue=queue))
else:
from warnings import warn
warn("No SVM support, returning a CL buffer-based memory pool")
Copy link
Contributor

Choose a reason for hiding this comment

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

Tell the user what aspect is missing (SVM for the device, vs missing newfangled SVM in pyopencl).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 9bc4e69

Copy link
Contributor

@inducer inducer left a comment

Choose a reason for hiding this comment

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

LGTM with more detailed diagnostics.

@matthiasdiener matthiasdiener merged commit 79c5559 into main Aug 16, 2022
@matthiasdiener matthiasdiener deleted the svm branch August 16, 2022 01:08
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

Successfully merging this pull request may close these issues.

4 participants