Support Numba Runtime in RBC #531
Conversation
rbc/nrt.py
Outdated
| # MemInfo struct | ||
| MemInfo_t = ir.global_context.get_identified_type('Struct.MemInfo') | ||
| # First argument ought to be "atomic_i64_t" instead of i64 | ||
| MemInfo_t.set_body(i64, i8p, i8p, i8p, i64, i8p) |
There was a problem hiding this comment.
Replace the first i64 by atomic_i64_t
| with otherwise: | ||
| # there is a specific case where ptr is NULL and doesn't | ||
| # crash Numba | ||
| bb_else = builder.basic_block | ||
| data_null = NULL |
There was a problem hiding this comment.
Document this odd corner case
…into guilhermeleobas/nrt
There was a problem hiding this comment.
Overall looks good, this is awesome stuff @guilhermeleobas!
I have a few questions/comments, mostly related to the API of the NRT functions. Will wait for your feedback on those before giving explicit approval.
| def ctor(lst): | ||
| sz = len(lst) | ||
| arr = Array(sz, dtype) | ||
| for i in range(sz): | ||
| arr[i] = lst[i] | ||
| return arr |
There was a problem hiding this comment.
Depending on how complicated the IR becomes, there's a chance that LLVM will optimize it on its own with -memcpyopt: https://llvm.org/docs/Passes.html#memcpyopt-memcpy-optimization
rbc/nrt.py
Outdated
| def define_NRT_Reallocate(self, builder, args): | ||
| with self.nrt_debug_ctx(): | ||
| # Because the previously reallocated memory might be freed by | ||
| # realloc, RBC cannot call it in this function. Therefore, instead | ||
| # of reallocation, RBC allocates a new block of memory and returns | ||
| # it to the caller. The caller is responsible for copying memory | ||
| # from the old buffer into the most recent allocated one." | ||
| [ptr, size] = args | ||
| new_ptr = builder.call(self.NRT_Allocate_External, [size, NULL]) | ||
| self.NRT_Debug("ptr=%p size=%zu -> new_ptr=%p", ptr, size, new_ptr) | ||
| builder.ret(new_ptr) |
There was a problem hiding this comment.
Is there any reason why realloc can't just be a wrapper around malloc + memcpy then? That would remove the weirdness about the caller being responsible for copying, which isn't the case for realloc. I think it's preferable to keep to the original function's API as much as possible.
There was a problem hiding this comment.
For that to work, RBC would need to know somehow the size of the previous allocated buffer, and this info is not available in NRT_Reallocate.
There was a problem hiding this comment.
You can use malloc_usable_size for determining the size of the malloc-allocated buffer. It would be Linux specific function though, see https://stackoverflow.com/questions/1281686/determine-size-of-dynamically-allocated-memory-in-c for Windows and Mac equivalents. Recall, heavydb can be built on Linux as well as on Windows.
It would be preferable to keep the original realloc API. Could you create an issue about this and emit an (only-once) warning in define_NRT_Reallocate call about the current behavior with a reference to the issue?
There was a problem hiding this comment.
See also NRT_Reallocate_NoCopy suggestion below. This would allow postponing resolving the realloc API issue.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Current CI failures are not related to this PR. |
|
Pinging @mflaxman10 for visibility |
There was a problem hiding this comment.
Have a number of nits and questions.
Otherwise, it is an amazing PR, thanks, @guilhermeleobas!
rbc/nrt.py
Outdated
| def define_NRT_Reallocate(self, builder, args): | ||
| with self.nrt_debug_ctx(): | ||
| # Because the previously reallocated memory might be freed by | ||
| # realloc, RBC cannot call it in this function. Therefore, instead | ||
| # of reallocation, RBC allocates a new block of memory and returns | ||
| # it to the caller. The caller is responsible for copying memory | ||
| # from the old buffer into the most recent allocated one." | ||
| [ptr, size] = args | ||
| new_ptr = builder.call(self.NRT_Allocate_External, [size, NULL]) | ||
| self.NRT_Debug("ptr=%p size=%zu -> new_ptr=%p", ptr, size, new_ptr) | ||
| builder.ret(new_ptr) |
There was a problem hiding this comment.
See also NRT_Reallocate_NoCopy suggestion below. This would allow postponing resolving the realloc API issue.
| # [X] set.remove | ||
| # [X] set.symmetric_difference | ||
| # [X] set.symmetric_difference_update | ||
| # [ ] set.union |
There was a problem hiding this comment.
What blocks supporting set.union?
There was a problem hiding this comment.
It doesn't seem to work in Numba:
self = <numba.core.typeinfer.TypeInferer object at 0x7ff2e5965f30>, raise_errors = True
def propagate(self, raise_errors=True):
newtoken = self.get_state_token()
oldtoken = None
# Since the number of types are finite, the typesets will eventually
# stop growing.
while newtoken != oldtoken:
self.debug.propagate_started()
oldtoken = newtoken
# Errors can appear when the type set is incomplete; only
# raise them when there is no progress anymore.
errors = self.constraints.propagate(self)
newtoken = self.get_state_token()
self.debug.propagate_finished()
if errors:
if raise_errors:
force_lit_args = [e for e in errors
if isinstance(e, ForceLiteralArg)]
if not force_lit_args:
> raise errors[0]
E numba.core.errors.TypingError: Failed in nopython mode pipeline (step: nopython frontend)
E Invalid use of BoundFunction(set.union for set(int64)) with parameters (set(Literal[int](0)))
E
E During: resolving callee type: BoundFunction(set.union for set(int64))
E During: typing of call at /home/guilhermeleobas/git/rbc/rbc/tests/heavydb/test_nrt_set.py (84)
E
E
E File "rbc/tests/heavydb/test_nrt_set.py", line 84:
E def test_set(t, method):
E <source elided>
E elif method_str == 'union':
E return len(s.union({0})) == 6
E ^
../../miniconda3/envs/rbc/lib/python3.10/site-packages/numba/core/typeinfer.py:1086: TypingError
This PR rewrites the Numba Runtime using LLVM.