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

lower_constant and lower_cast should probably return new references #4494

Open
asodeur opened this issue Aug 30, 2019 · 3 comments
Open

lower_constant and lower_cast should probably return new references #4494

asodeur opened this issue Aug 30, 2019 · 3 comments

Comments

@asodeur
Copy link
Contributor

asodeur commented Aug 30, 2019

Feature request

Currently @lower_constant and @lower_cast expect the implementation to return borrowed references. This makes it tricky to implement lowering constants and casts for memory managed types as you cannot just use native constructors (if available). For casting there might even be edge cases that cannot be implemented at all (#3492).

It would be great if the following naive implementation strategy for lowering constants and casts worked. Assume we have an existing (maybe memory managed) Numba type AType and a new extension type BType with a native constuctor B(a: A).

Lowering a constant B might look like:

@lower_constant(BType)
def lower_constant_b(context, builder, typ, pyval):
    a = pyval.a  # assuming B has an attribute a and constant AType can be lowered
    def lower_b_impl():
        return B(a) 
    return context.compile_internal(builder, lower_b_impl, typ(), [])  # returns new ref

Casting AType to BType could be implemented like

@lower_cast(types.Number, UnitType)
def cast_a_to_b(context, builder, fromty, toty, val):
    def cast_a_b_impl(a):
        return B(a)
    return context.compile_internal(builder, lower_b_impl, totyp(fromty), [val])  # returns new ref

Withe the current refcounting conventions the implementations above will leak memory. In some cases you can avoid the memory leak by punting on the refcount pruning pass and simply call context.nrt.decref on the result of compile_internal. However, this will not work in general (for example casting tuples will not work) and crash the interpreter.

@stuartarchibald
Copy link
Contributor

Thanks for the report. For context, some gitter chat: https://gitter.im/numba/numba?at=5d652f5e07d1ff39f8912692

Am wondering if something like the RefType indication in the decorator might help as per http://numba.pydata.org/numba-doc/latest/reference/deprecation.html#id18, but this may be too simplistic.

@asodeur
Copy link
Contributor Author

asodeur commented Sep 2, 2019

Based on your suggestion on the mailing list I implemented just that and opened PR #4506. Still trying to figure-out if there is a way to get my code working w/o changing the conventions in the core but does not look promising at the moment.

@stuartarchibald
Copy link
Contributor

This was discussed at the core developer meeting on 3rd Sept 2019. General consensus was:

  1. That at Numba IR level implicit cast should not happen and in fact making all casts explicit is neater/easier to reason about/consistent. Users should see no behavioural change.
  2. Reuse of existing ir.Expr.cast should be fine.
  3. Adding RefType or similar for safety and ensuring it's backwards compatible is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants