Skip to content

Remove @overload_glue for NumPy allocators. - #7999

Merged
sklam merged 3 commits into
numba:mainfrom
stuartarchibald:wip/reduce_glue2
Jun 22, 2022
Merged

sklam merged 3 commits into
numba:mainfrom
stuartarchibald:wip/reduce_glue2

Conversation

@stuartarchibald

@stuartarchibald stuartarchibald commented Apr 21, 2022 •

Copy link
Copy Markdown
Contributor

Turns these functions into true @overloads.

  • np.empty
  • np.empty_like
  • np.ones
  • np.ones_like
  • np.zeros
  • np.zeros_like

also adds a _zero_fill method to the types.Array type to memset
the array's memory region to zero, this is unchecked and for
convenience only.

Also converts the .imag and .real attributes of types.Array to use @overload_attribute.

Turns these functions into true `@overloads`.

* np.empty
* np.empty_like
* np.ones
* np.ones_like
* np.zeros
* np.zeros_like

also adds a `_zero_fill` method to the `types.Array` type to memset
the array's memory region to zero, this is unchecked and for
convenience only.
Comment thread numba/np/arrayobj.py
@lower_getattr(types.Array, "imag")
def array_imag_part(context, builder, typ, value):
@intrinsic
def _force_readonly(tyctx, arr):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Move this to numba.np.unsafe.ndarray?

@stuartarchibald
stuartarchibald marked this pull request as ready for review April 26, 2022 17:09
@stuartarchibald

Copy link
Copy Markdown
Contributor Author

Note to reviewers: This patch is largely verbatim code motion, moving explicit typing templates into the typing part of @overloads, and then refactoring lowering implementations into @intrinsics called by @overload, or higher level APIs where possible.


self.assertIn('No match', excstr)
msg = (f"If np.{self.pyfunc.__name__} dtype is a string it must be a "
"string constant.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! better error message

Comment thread numba/np/arrayobj.py Outdated
array = arrayty(cgctx, builder, llargs[0])
# make readonly hack
parent = array._datamodel.get_type('parent')
setattr(array, 'parent', Constant(cgctx.get_value_type(parent), None))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does it need to set parent to NULL? The only place this is used is in ary.imag for real domain array. The .parent is always NULL in that case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I vaguely recall adding this with view of trying provide a "general" way to set the readonly bit, was wondering (here: #7999 (comment)) whether this ought to be a utility function? Am fine to remove it or move it into numba.np.unsafe.ndarray.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's remove it from _force_readonly. I don't think this is the way to do it. The boxer is already setting the writeable flag according to the array readonly/mutable property:

writable = self.context.get_constant(types.int32, int(aryty.mutable))
. The "parent" affects the OWNDATA flag and it is a different concern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TBH, I'd rather that this didn't exist at all, but this sort of thing fails because the return type of the impl doesn't match the declared signature:

from numba import njit
from numba.core.extending import overload
import numpy as np

def foo(x):
    pass

@overload(foo)
def ol_foo(x):
    def impl(x):
        return x
    return x.copy(readonly=True)(x), impl

@njit
def call_foo(x):
    return foo(x)

tmp = np.arange(5.)
print(call_foo(tmp))

so the intrinsic is needed to fix it. I'll go fix the patch WRT the above.

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 3 - Ready for Review labels Jun 20, 2022

@sklam sklam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#7999 (comment) is the only thing

@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jun 21, 2022
@stuartarchibald

Copy link
Copy Markdown
Contributor Author

#7999 (comment) is the only thing

Addressed in d69a6ac

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jun 22, 2022

@sklam sklam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the patch!

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jun 22, 2022
@sklam
sklam merged commit d4e19b2 into numba:main Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants