Expose LocalAccessor as kernel argument type #1991
Conversation
|
View rendered docs @ https://intelpython.github.io/dpctl/pulls/1991/index.html |
|
Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_518 ran successfully. |
|
Array API standard conformance tests for dpctl= ran successfully. |
ndgrigorian
requested review from
antonwolfy,
vlad-perevezentsev and
vtavana
as code owners
|
Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_540 ran successfully. |
dpctl/_sycl_queue.pyx
Outdated
| self.lacc.dpctl_type_id = _arg_data_type._UINT64_T | ||
| elif type == 'f4': | ||
| self.lacc.dpctl_type_id = _arg_data_type._FLOAT | ||
| elif type == 'f8': |
There was a problem hiding this comment.
Given that the FP8 format is gaining traction and there is a micro-scaling FP4 format, I find the naming here confusing.
Why don't we follow the naming scheme of numpy for types?
There was a problem hiding this comment.
Which naming scheme do you mean? NumPy does accept kind + size in bytes, unless you mean using f for float32 specifically and d for float64 specifically.
I think we would preferably support both, so it would be a good change to make it more flexible.
There was a problem hiding this comment.
I was thinking about the name returned by dtype, like "float64" in this example here:
a = np.array([1, 2], dtype=float)
a.dtype == np.dtype(np.float64)
a.dtype == np.float64
a.dtype == float
a.dtype == "float64"
a.dtype == "d"
But you're right, the numpy scheme for specifying types matches what we have here, so we can just keep that.
It also aligns with what is used in other places in DPCTL, so makes sense to keep it consistent.
There was a problem hiding this comment.
I do think it's sensible to support addition dtype strings ("float32", "float64") and possibly NumPy's char names ("f", "d").
It may even be sensible to expose the dpctl dtype enumerators in the dpctl namespace as enums (not dtype objects like in tensor) and add them as possible arguments, but this may be better saved for a subsequent PR. Another thought being that this could be re-used for a typed work_group_memory overload
There was a problem hiding this comment.
Yes, an enumeration for the possible dtypes would be useful, if we need it in multiple places, such as typed work_group_memory overload. But I agree, this can be done in a follow-up PR.
…uments LocalAccessor(ndim, elemental_type_str, dim0, dim1, dim2) The elemental type can be one of the following: "i1", "u1", "i2", "u2", "i4", "u4", "i8", "u8", "f4", "f8"
Fixes conflict with built-in `type`
ndgrigorian
force-pushed
the
add-md-local-accessor
branch
from
ff43ac7 to
e1e4b00
Compare
|
Array API standard conformance tests for dpctl=0.20.0dev0=py310h93fe807_11 ran successfully. |
ndgrigorian
force-pushed
the
add-md-local-accessor
branch
from
e1e4b00 to
949571b
Compare
|
Array API standard conformance tests for dpctl=0.20.0dev0=py310h93fe807_11 ran successfully. |
Also improve messages in errors
ndgrigorian
force-pushed
the
add-md-local-accessor
branch
from
949571b to
f35e483
Compare
|
Array API standard conformance tests for dpctl=0.20.0dev0=py310h93fe807_11 ran successfully. |
This PR adds
_md_local_accessorCython struct in_backend.pxdthat ties toMDLocalAccessorstruct defined in libsyclinterface, and which is to be used to indicate local_accessor kernel argument type (see test file in syclinterface library)._sycl_queue.pyxthen definesLocalAccessortype, constructible usingLocalAccessor(ndim, typestr, dim0, dim1, dim2)constuctor. Thendimargument represents accessor dimensionality (only values 1, 2, or 3 are supported). Thetypestrcan be"i1","u1","i2","u2","i4","u4","i8","u8","f4", or"f8"`.