IntelPython / dpctl

Python SYCL bindings and SYCL-based Python Array API library
https://intelpython.github.io/dpctl/
Apache License 2.0
102 stars 29 forks source link

Check pointer alignment when copying from strided array to C-contiguous array #1890

Closed ndgrigorian closed 2 weeks ago

ndgrigorian commented 2 weeks ago

This PR resolves https://github.com/IntelPython/dpctl/issues/1887

When using sub-group loads and stores, certain alignment of pointers is required. Copies to C-contiguous memory were not properly checking alignment, which would lead to incorrect results.

Before, using the example in #1887:

import dpctl.tensor as dpt

padded = dpt.zeros((8, 23), dtype=dpt.int64)
value = dpt.asarray([[0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])
right_slice = (slice(1, None, None), Ellipsis)
padded[right_slice] = value
print(padded)
#usm_ndarray([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])

with this change:

import dpctl.tensor as dpt

padded = dpt.zeros((8, 23), dtype=dpt.int64)
value = dpt.asarray([[0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])
right_slice = (slice(1, None, None), Ellipsis)
padded[right_slice] = value
print(padded)
# expected result
#usm_ndarray([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
#             [0, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])
ndgrigorian commented 2 weeks ago

@vtavana I've made a change that fixes the copy issue. Please test it out.

I will want to refactor the code a bit to reduce redundancy before this is merged, but this resolved the problem for me locally.

github-actions[bot] commented 2 weeks ago

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. :crossed_fingers:

github-actions[bot] commented 2 weeks ago

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_198 ran successfully. Passed: 895 Failed: 0 Skipped: 119

github-actions[bot] commented 2 weeks ago

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_198 ran successfully. Passed: 895 Failed: 0 Skipped: 119

coveralls commented 2 weeks ago

Coverage Status

coverage: 87.705%. remained the same when pulling 0bed4aa39502e90ada4b5c987c9ada5dabb51791 on fix-gh-1887 into 79bc274c9997f5db9bae7f095f100d68f645c0be on master.

github-actions[bot] commented 2 weeks ago

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_200 ran successfully. Passed: 894 Failed: 1 Skipped: 119

github-actions[bot] commented 2 weeks ago

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_203 ran successfully. Passed: 894 Failed: 1 Skipped: 119

vtavana commented 2 weeks ago

The relevant tests for dpnp.pad have now successfully passed with these updates.

github-actions[bot] commented 2 weeks ago

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_204 ran successfully. Passed: 894 Failed: 1 Skipped: 119