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

Kernel signature uniformisation #11

Open
StRigaud opened this issue Jan 27, 2021 · 7 comments
Open

Kernel signature uniformisation #11

StRigaud opened this issue Jan 27, 2021 · 7 comments
Assignees

Comments

@StRigaud
Copy link
Member

Though most of the implementation follows a certain logic, some signature different and varies, by little or by a lot. Both in parameters name and order. Some example: maximum_z_projection_x, absolute_2d_x, and add_image_and_scalar_2d_x

I believe it could be nice to uniform as much as possible the kernel signature, to make it a generic as possible, by defining a parameter name nomenclature and order. This would simplify certain implementation aspect on the C++ side (and Python?), improve scalability, and facilitate most of the repetitive implementation (auto-generation?). And this could also facilitate the writing of a documentation raised in #10 .

I would be ok to do it, with anyone who also want to suffer with me, but we need first to have a good signature rule. I am not aware enough yet with all the kernels and the specific cases but here are some first suggestions from what I could see:

  1. enforce the input and output parameter to be respectively src and dst
  2. enforce the first parameter to be dst, and the second parameter to be src. This is mainly because dst is always present (e.g. set_2d_x)
  3. if multiple input or output required, the addition of a numeration, starting from 2, e.g. src, src2, src3 and dst, dst2, dst3
  4. enforce order priority to first I/O buffer, then buffer parameters, and finally scalar parameters
  5. the scalar parameter name have less impact as they do not require a push or pull to the GPU side, hence they could keep the meaning full name (e.g. sigma for Gaussian blur).

This would lead to this kind of signature:

  • __kernel do_something_2d ( dst, src )
  • __kernel do_something_2d ( dst, src, src2, scalar, scalar2 )

And the fun part begins with multiple input and output as it raise two options:

  1. __kernel do_something_2d ( dst, dst2, src, src2, scalar, scalar2 )
  2. __kernel do_something_2d ( dst, src, dst2, src2, scalar, scalar2 )
@haesleinhuepf
Copy link
Member

Awesome idea @StRigaud ! Just some questions:

  • Names like "src_label" and "dst_touch_matrix" are ok, or?
  • Would it be ok from your side if we had input-image parameters first before output-image parameters and finally numberic and boolean parameters? We could then have the same order on all levels (OpenCL, Macro, Python, ...). It's not a must, but I'm afraid the workload will be huge if we change this order.

@StRigaud
Copy link
Member Author

* Names like "src_label" and "dst_touch_matrix" are ok, or?

Well, the idea is to avoid such specific name tag when we can. Has src is an input tag, src_label look like input and should be simply named src (or src2 if second).
However, we could consider also buffer/image to be something else than an Input or Output, and define naming tag like label, map, or flag. Though the idea is that a name tag does not exist only for one particular kernel, and has a certain re-usability.

A down side is that we loose information from the parameter naming, but we gain a heavy generalization on the kernel call, and we can have still this information at the Python or C++ level.

* Would it be ok from your side if we had input-image parameters first before output-image parameters

Honestly, I am fine with it. I proposed dst first because in all the kernel existing, it is the parameter that we always have, and nothing forbid to keep the input first in CLIj, Python, C++ ...
The ocl Kernel are completely hidden to the users, or the front-end developer, hence nothing forbid us to have a rule at the kernel level which differ from the top API level

@haesleinhuepf
Copy link
Member

I proposed dst first because in all the kernel existing, it is the parameter that we always have

Yeah, that's because I was lazy. However, I think code quality / readability would profit from proper variable names. They still should have some "src" and "dst" in it, because in OpenCL1.2 images are not supported to be read-write on some target platforms.

The ocl Kernel are completely hidden to the users, or the front-end developer, hence nothing forbid us to have a rule at the kernel level which differ from the top API level

Yes. And I'm happy to write down these rules. Maybe here would be the right place.

@StRigaud
Copy link
Member Author

I don't think we should change the initial src / dst.

An input is an input, whatever is a label image or a distance map. The kernel operation set the context of the input and output. An example is in ITK where all input and output are called by a SetInput and GetOutput methods, whatever are the I/O. Hence I don't think it should have a more specialise name, at least for the main Input and Output.

The side input like flags, or label maps could have specific names though as they would concern specific kernels. And we need exception to the rules otherwise my French side will be triggered 😁

@StRigaud
Copy link
Member Author

StRigaud commented Feb 2, 2021

The same idea apply on the kernel name. Nearly all of them are already fitting a format:

kernel_doing_something[_specifics][_nd]_x.cl with _nd the dimension tag, and _specifics corresponding to specifics method like separable or interpolate.

examples:

  • absolute_2d_x.cl
  • mean_separable_2d.cl

though some would be modify as affine_transform_2d_interpolate_x.cl into affine_transform_interpolate_2d_x.cl

@StRigaud
Copy link
Member Author

StRigaud commented Feb 4, 2021

Here is an excel sheet parsed from all the kernels as:

  • kernel filename
  • kernel name
  • parameters.

And for each parameters I subdivided in 3 columns:

  • the tag (src, dst, scalar, ...).
  • the method (associated name in the API, for Set/Get methods in C++, or argument name in python) <-- empty for now.
  • the type (float, int, IMAGE_src_TYPE,...) where IMAGE_src_TYPE would correspond to Buffer or Image2d, image3d.

I let you enjoy the diversity of @haesleinhuepf imagination at naming stuff.

For now, the excel is in view only, we could use it for a base of discussion, highlight issues, and to track modifications.

@haesleinhuepf haesleinhuepf self-assigned this Nov 18, 2021
@StRigaud
Copy link
Member Author

TODO for @haesleinhuepf

  • Create clesperanto kernel empty branch
  • push readme
  • push rules of kernels
  • push first draft of ready to-be-clean kernels
  • push cleaned version of preamble.cl

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

2 participants