KernelIntrinsics API

[christiangnrd]

The goal is to allow for kernels to be written without relying on KernelAbstractions macros

See #562 for initial discussion

@vchuravy @maleadt

[github-actions]

Your PR requires formatting changes to meet the project's style guidelines.
Please consider running Runic (git runic main) to apply these changes.

Click here to view the suggested changes.

diff --git a/src/pocl/backend.jl b/src/pocl/backend.jl
index 055e1c7d..abf0f803 100644
--- a/src/pocl/backend.jl
+++ b/src/pocl/backend.jl
@@ -149,9 +149,9 @@ end
 function (obj::KI.Kernel{POCLBackend})(args...; numworkgroups = 1, workgroupsize = 1)
     KI.check_launch_args(numworkgroups, workgroupsize)
 
-    local_size = (workgroupsize..., ntuple(_->1, 3-length(workgroupsize))...,)
+    local_size = (workgroupsize..., ntuple(_ -> 1, 3 - length(workgroupsize))...)
 
-    numworkgroups = (numworkgroups..., ntuple(_->1, 3-length(numworkgroups))...,)
+    numworkgroups = (numworkgroups..., ntuple(_ -> 1, 3 - length(numworkgroups))...)
     global_size = local_size .* numworkgroups
 
     event = obj.kern(args...; local_size, global_size)

[vchuravy]

Can you rebase?

[vchuravy]

@christiangnrd do you think we need a lower-level kernel launch interface? Otherwise the three-dimensional indices would be superflous.

[christiangnrd]

do you think we need a lower-level kernel launch interface?

I'd been thinking about that and I think so. Would something that, assuming you wrote the whole kernel with KernelIntrinsics, could be used interchangeably with the backend's kernel launch macro (@cuda, @metal, etc.) make sense/be feasible?

[vchuravy]

Nice! Maybe we can also add some simple shuffle operations?

[vchuravy]

This is technically an ABI break, which I had avoided so far.

[christiangnrd]

Is ca11482 sufficient or do we still need to require backends to take in a third unused argument on the KI side?

[christiangnrd]

I'm thinking we should either directly rename KernelIntrinsics to KI or provide the alias.

[vchuravy]

I am okay with providing the alias!

[vchuravy]

One additional thing to consider is what intrinsics do we need to implement something like #559

Can we add some primitive shuffles? Or add a test kernel that implements a reduction correctly?

[christiangnrd]

One additional thing to consider is what intrinsics do we need to implement something like #559

Can we add some primitive shuffles? Or add a test kernel that implements a reduction correctly?

I think this should be a follow-up PR since it'll probably generate quite a bit of discussion, but I feel like shfl_down and a check for support taking into account backend and type may be all that's needed?

Otherwise, I believe the only things left to resolve for this PR are the module docstring, #635 (comment), and I'm not sure if we should at least start considering dynamic local memory API to make sure we don't need to release another breaking version or if you think the current api can definitely fit the addition of local memory.

[maleadt]

I am okay with providing the alias!

Do we really need to, given the import KernelIntrinsics as KI feature?

[christiangnrd]

Do we really need to, given the import KernelIntrinsics as KI feature?

I felt like we might as well include it since it'll be written so much, and I found fully qualifying KernelIntrinsics for every KI function mention in the docstrings was distracting, but also didn't want to write KI since users new to the package would be confused wondering where it came from.

I don't feel that strongly about this though so I'll defer to you for the final decision