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

Adds Range and NdRange as supported types in numba_dpex.dpjit. #1148

Merged
merged 6 commits into from
Oct 2, 2023

Conversation

diptorupd
Copy link
Contributor

@diptorupd diptorupd commented Sep 28, 2023

  • Have you provided a meaningful PR description?
    • The numba_dpex.core.kernel_interface.indexers.Range and
      NdRange classes can now be used inside a numba_dpex.dpjit
      decorated function, as arguments to a dpjit decorated function,
      and returned from a dpjit decorated function.
    • Defines a datamodel, type class, overloads for the constructors,
      box, and unbox functions for both classes.
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • If this PR is a work in progress, are you filing the PR as a draft?
    The PR should be merged post 0.21.3 tag

@diptorupd diptorupd requested a review from ZzEeKkAa September 28, 2023 03:59
@diptorupd diptorupd marked this pull request as draft September 28, 2023 03:59
@diptorupd diptorupd force-pushed the feature/range_ndrange_type_support branch from 66d6bd3 to b0a2d1e Compare September 29, 2023 02:43
@diptorupd diptorupd marked this pull request as ready for review September 29, 2023 03:44
@diptorupd diptorupd force-pushed the feature/range_ndrange_type_support branch 2 times, most recently from 69330d8 to f3b7d26 Compare September 29, 2023 03:46
Copy link
Contributor

@ZzEeKkAa ZzEeKkAa left a comment

Choose a reason for hiding this comment

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

Overall looks good. I was trying to understand what's going on here, so most of the comments is more to get better understanding, that request to changes something. Also NDRange concept still unclear for me.

numba_dpex/core/kernel_interface/indexers.py Outdated Show resolved Hide resolved
numba_dpex/core/types/range_types.py Outdated Show resolved Hide resolved
with cgutils.early_exit_if_null(c.builder, stack, dim2_obj):
c.builder.store(fail_obj, ret_ptr)

class_obj = c.pyapi.unserialize(c.pyapi.serialize_object(Range))
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, lack of knowledge, could you explain why do we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First a mea culpa, I have no idea! Purely "monkey see, monkey do" from the Numba extension guide.

But, I dug up what it does. Here is the paper trail:

https://github.com/numba/numba/blob/9f74128def1ca51deebfadecc663f0b80f641c57/numba/core/pythonapi.py#L1349

https://github.com/numba/numba/blob/9f74128def1ca51deebfadecc663f0b80f641c57/numba/core/pythonapi.py#L1402

https://github.com/numba/numba/blob/9f74128def1ca51deebfadecc663f0b80f641c57/numba/_helperlib.c#L1058

https://docs.python.org/3/c-api/bytes.html#c.PyBytes_FromStringAndSize

My superficial reading of what is happening here is that for every registered type Numba is maintaining a cache of how to serialize and unserialize an object of that type using the Python pickle module. It is a functionality I believe was designed mainly for on-disk caching of compiled functions and that involves being able to box and unbox argument types. So, we are using that API to call the CPython PyBytes_FromStringAndSize to get the PyObject structure that represents a Range object in this case. Once we have that we are next doing a PyObject_CallFunctionObjArgs to call the constructor for that object.

We might need a better CPython expert like @oleksandr-pavlyk for further explanation.

"""
Convert a native range structure to a Range object.
"""
ret_ptr = cgutils.alloca_once(c.builder, c.pyapi.pyobj)
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right that this one creates reference on the created object, that's why we don't have to increase reference manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ret_ptr is the PyObject* into which the res (Range) object to be stored is stored. Since it is a new object that gets returned from Numba to CPython, a incref is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, how does it getting cleaned then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a PyObject is created the ref count should be 1. Once Numba returns the object to CPython, then CPython will track the lifetime of the object, and decref once the object is out of scope and dead. The garbage collector will free it up at that point.

Diptorup Deb added 5 commits September 30, 2023 11:33
    - The numba_dpex.core.kernel_interface.indexers.Range and
      NdRange classes can now be used inside a numba_dpex.dpjit
      decorated function, as arguments to a dpjit decorated function,
      and returned from a dpjit decorated function.
    - Defines a datamodel, type class, overloads for the constructors,
      box, and unbox functions for both classes.
@diptorupd diptorupd force-pushed the feature/range_ndrange_type_support branch from 38ee434 to af0208d Compare September 30, 2023 16:35
    - A static variable Range.UNDEFINED_DIMENSION is now used
      instead of the magic -1 value to indicate that the dimension
      extent is undefined.
    - An ndim_obj need not be created when boxing a Range.
@diptorupd diptorupd force-pushed the feature/range_ndrange_type_support branch from af0208d to 7e44902 Compare September 30, 2023 16:37
@diptorupd
Copy link
Contributor Author

Also NDRange concept still unclear for me.

https://enccs.github.io/sycl-workshop/expressing-parallelism-nd-range/

Copy link
Contributor

@ZzEeKkAa ZzEeKkAa left a comment

Choose a reason for hiding this comment

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

Still not 100% sure if I competent enough to put approval if it is good or not, but from my perspective it LGTM)

@diptorupd diptorupd merged commit cd5332f into main Oct 2, 2023
22 checks passed
@diptorupd diptorupd deleted the feature/range_ndrange_type_support branch October 2, 2023 23:05
github-actions bot added a commit that referenced this pull request Oct 2, 2023
…support

Adds Range and NdRange as supported types in numba_dpex.dpjit. cd5332f
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.

3 participants