Check pointer alignment when copying from strided array to C-contiguous array by ndgrigorian · Pull Request #1890 · IntelPython/dpctl · GitHub
Skip to content

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

Merged
ndgrigorian merged 7 commits into
masterfrom
fix-gh-1887
Nov 13, 2024
Merged

Check pointer alignment when copying from strided array to C-contiguous array#1890
ndgrigorian merged 7 commits into
masterfrom
fix-gh-1887

Conversation

@ndgrigorian

@ndgrigorian ndgrigorian commented Nov 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR resolves #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]])
  • Have you provided a meaningful PR description?
  • 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?
  • Have you checked performance impact of proposed changes?
  • Have you added documentation for your changes, if necessary?
  • Have you added your changes to the changelog?
  • If this PR is a work in progress, are you opening the PR as a draft?

@ndgrigorian ndgrigorian requested a review from vtavana November 13, 2024 00:33
@ndgrigorian

ndgrigorian commented Nov 13, 2024

Copy link
Copy Markdown
Collaborator Author

@github-actions

github-actions Bot commented Nov 13, 2024 •
edited
Loading

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Nov 13, 2024

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Nov 13, 2024

Copy link
Copy Markdown

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

@coveralls

coveralls commented Nov 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Coverage Status

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

@github-actions

github-actions Bot commented Nov 13, 2024

Copy link
Copy Markdown

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

@ndgrigorian ndgrigorian marked this pull request as ready for review November 13, 2024 07:12
@oleksandr-pavlyk oleksandr-pavlyk mentioned this pull request Nov 13, 2024
Merged
8 tasks
oleksandr-pavlyk and others added 4 commits November 13, 2024 09:47
@oleksandr-pavlyk
Tweaks to copy_as_contiguous CContig functor
04fd35c 
1. Save common subexpressions to variables
2. Sub-group size type changed to uint16 (from uint32)
3. sg.get_local_range() replaced with sg.get_max_local_range()

   This is safe to do since work-group size is chosen to be
   a multiple of sub-group size for all possile choices
   of sub-group size (1, 8, 16, 32, 64)

4. Simplified computation of base value in generic branch for
   complex types, or when sg_load is disabled, to avoid a
   division (and left a comment)
@oleksandr-pavlyk
Added static assert to verify that n_vecs * vec_sz fits in uint8_t
18f7ea0 
Also reordered template parameters vec_sz, n_vecs for consistency
with the wide code-base.
@oleksandr-pavlyk
Merge pull request #1891 from IntelPython/contribution-to-fix-gh-1887
2ea5dfd 
Contribution to fix gh 1887
@ndgrigorian
Add fix for gh-1887 to CHANGELOG
0bed4aa
@github-actions

github-actions Bot commented Nov 13, 2024

Copy link
Copy Markdown

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

@vtavana

vtavana commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Nov 13, 2024

Copy link
Copy Markdown

oleksandr-pavlyk
oleksandr-pavlyk approved these changes Nov 13, 2024
View reviewed changes

@oleksandr-pavlyk oleksandr-pavlyk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT!

@ndgrigorian ndgrigorian merged commit dd2812f into master Nov 13, 2024
@ndgrigorian ndgrigorian deleted the fix-gh-1887 branch November 13, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vtavana vtavana Awaiting requested review from vtavana
1 more reviewer
@oleksandr-pavlyk oleksandr-pavlyk oleksandr-pavlyk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect result using slice with usm_ndarray on GPU devices not supporting fp64

4 participants

@ndgrigorian @coveralls @vtavana @oleksandr-pavlyk