-
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
Fix/atomics #346
Fix/atomics #346
Conversation
I will review this in detail later. One note for all the enums, please document them in detail. The links to Dpcpp source is not sufficient, moreover the links are subject to change if Dpcpp code changes. |
numba_dppy/config.py
Outdated
|
||
# Activate Native floating point atomcis support for supported devices. | ||
# Requires llvm-spirv supporting the FP atomics extensio | ||
NATIVE_FP_ATOMICS = _readenv("NUMBA_DPPY_ACTIVATE_NATIVE_FP_ATOMICS", int, 0) |
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 better to place more general part of name first: ATOMICS_FP_NATIVE
numba_dppy/ocl/oclimpl.py
Outdated
from numba_dppy.ocl.atomics import atomic_helper | ||
import dpctl | ||
from numba_dppy.config import NATIVE_FP_ATOMICS |
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.
Use isort if possible for sorting imports.
LINK_ATOMIC = 111 | ||
LLVM_SPIRV_ARGS = 112 |
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.
Magic numbers?
numba_dppy/spirv_generator.py
Outdated
|
||
if llvm_spirv_root == "": | ||
# try to find ONEAPI root | ||
possible_oneapi_roots = ["/opt/intel/oneapi", "$A21_SDK_ROOT"] |
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 cannot have these in our code-base. Users should set it themselves using envars.
@@ -62,5 +62,9 @@ def _readenv(): ... | |||
|
|||
FALLBACK_ON_CPU = _readenv("NUMBA_DPPY_FALLBACK_ON_CPU", int, 1) | |||
|
|||
# Activate Native floating point atomcis support for supported devices. | |||
# Requires llvm-spirv supporting the FP atomics extensio | |||
NATIVE_FP_ATOMICS = _readenv("NUMBA_DPPY_ACTIVATE_ATOMCIS_FP_NATIVE", int, 0) |
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.
Please, make the same name in both places.
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.
Cython to use for auto update.
(https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_memory_ordering). | ||
|
||
For DPCPP implementation please refer: | ||
https://github.com/intel/llvm/blob/sycl-nightly/20210507/sycl/include/CL/sycl/ONEAPI/atomic_enums.hpp#L25 |
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 need protection for eliminate mismatch between DPCPP and our copy of enums.
|
||
long numba_dppy_atomic_add_i64_local(volatile __generic long *p, long val) { | ||
long found = *p; | ||
long expected; | ||
do { |
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.
Removed all integer functions because they work. Only floats needed.
This PR will add native floating-point atomics for GPUs. This work will require llvm-spirv translator that supports the
SPV_EXT_shader_atomic_float_add
extension to generate the native floating point atomic instruction.