search

inducer / pyopencl

OpenCL integration for Python, plus shiny features
http://mathema.tician.de/software/pyopencl
Other
1.06k stars 241 forks source link

Add events to ElementwiseKernel args #611

Closed alexfikl closed 2 years ago

alexfikl commented 2 years ago

This is what I was trying to ask today, i.e. if the input / output args of ElementwiseKernel get the event that gets generated. Currently they do not. Should they?

For context, this is about removing the manual event handling in boxtree for something like the GappyCopyAndMapKernel https://github.com/inducer/boxtree/blob/529e8d62f365350a80c9f16a0b12b9fb2c1a140e/boxtree/tools.py#L453-L509 which has a bunch of arguments.

inducer commented 2 years ago

Hmm, thanks for the reminder. #303 is related. One complexity here is that ElementwiseKernel (at least without additional help) has no way to find out which array is being written. Tthough, in keeping with #303, read-after-write and write-after-read are also real dependencies. Part of me feels that we should just unconditionally add events to everything (reads and writes), but then another part of me would like to start tackling #303 first, by separating read events and write events.

inducer commented 2 years ago

You closed this, but didn't leave a comment. Could you explain?

alexfikl commented 2 years ago

@inducer From your comment, it seemed like #303 is needed before anything useful can be done here. And adding events unconditionally could also be done in boxtree as a stopgap, so I didn't think this was needed.

Do you think it would be better to do something like that (maybe under a flag) for the various kernel classes in here?

inducer commented 2 years ago

I'm not sure it makes sense to clutter up boxtree with event management code. I think the desired goal state, from my perspective, would be to have array events split into read and write, and for that to be an effective change, for arguments to ElementwiseKernel to declare whether they're reading or writing. At that point, boxtree would need to be updated to declare read/write, and we could call this done. I'll reopen this to track that change. Sound fair?

inducer commented 2 years ago

Or... since this is a PR, maybe not. I'll reclose it and make an issue. Sorry about the back-and-forth.

inducer commented 2 years ago

Even better. I'll add this as an item under #303.

alexfikl commented 2 years ago

Sound fair?

Yeah, that sounds like a good plan to me!