-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add a target inside the experimental module #1213
Conversation
from numba_dpex.core.exceptions import ( | ||
ExecutionQueueInferenceError, | ||
UnsupportedKernelArgumentError, | ||
) | ||
from numba_dpex.core.pipelines import kernel_compiler | ||
from numba_dpex.core.types import DpnpNdArray | ||
|
||
from .target import dpex_exp_kernel_target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to avoid relative imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but in the past I have found relative imports to be the only way out of avoiding circular imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is okay for now then. We just need to rethink design to avoid circular imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree 👍
@@ -177,7 +179,7 @@ class KernelDispatcher(Dispatcher): | |||
|
|||
""" | |||
|
|||
targetdescr = dpex_kernel_target | |||
targetdescr = dpex_exp_kernel_target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see usage of targetdescr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed by the superclass: https://github.com/numba/numba/blob/a4664180ddc91e4c8a37dd06324f4e229a76df91/numba/core/dispatcher.py#L796
The variable is used in the super().__init__
: https://github.com/numba/numba/blob/a4664180ddc91e4c8a37dd06324f4e229a76df91/numba/core/dispatcher.py#L823
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my fault. It was confused, because it is defined on class level. I was expecting to see it in init with self.*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Add a target inside the experimental module 6f670d6
Have you provided a meaningful PR description?
Changes introduced in the experimental module should not have side effects on the
numba_dpex.core
components. For this reason, an experimental target is being introduced intonumba_dpex.experimental
. Any future typing or lowering changes for any experimental feature should be introduced here.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?