Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix coalesced access checks in matrix_vector_op #372

Merged
merged 9 commits into from
Nov 17, 2021
Merged

Fix coalesced access checks in matrix_vector_op #372

merged 9 commits into from
Nov 17, 2021

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented Nov 9, 2021

One of the conditions in test_aligned_access in linalg/matrix_vector_op.cuh was incorrect (ptr % elem_size should be zero, not otherwise). Due to that typo, matrixVectorOp function was never using vectorized load/store instructions.
This PR fixes the problem while also adding a new helper struct to simplify such checks in future.

@achirkin achirkin requested review from a team as code owners November 9, 2021 12:03
@github-actions github-actions bot added the cpp label Nov 9, 2021
@achirkin achirkin added non-breaking Non-breaking change bug Something isn't working enhancement New feature or request 3 - Ready for Review and removed cpp labels Nov 9, 2021
@achirkin
Copy link
Contributor Author

achirkin commented Nov 9, 2021

I've noticed this problem while investigating that normalizing the dataset using this op takes major fraction of time in the least squares algorithm (lstsq/eig). Just fixing that reduces the typical exec time of matrixVectorOp by a factor of x1.5-x2 in my tests. However, a highly specialized handwritten-kernel version reduces the time by a factor of x8-x10. So I will try to see if there is a room for improvements here.

@achirkin achirkin added the cpp label Nov 9, 2021
@achirkin achirkin changed the title Fix coalesced access checks in matrix_vector_op. Fix coalesced access checks in matrix_vector_op Nov 9, 2021
@dantegd
Copy link
Member

dantegd commented Nov 9, 2021

rerun tests

cjnolet
cjnolet requested changes Nov 15, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Looks good, just need a couple small unit tests for new utilities file

cpp/include/raft/pow2_utils.cuh Show resolved Hide resolved
@cjnolet
Copy link
Member

cjnolet commented Nov 15, 2021

rerun tests

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented Nov 15, 2021

rerun tests

@github-actions github-actions bot added the CMake label Nov 17, 2021
@achirkin achirkin changed the base branch from branch-21.12 to branch-22.02 November 17, 2021 17:27
@achirkin achirkin requested a review from a team as a code owner November 17, 2021 17:27
@cjnolet cjnolet changed the base branch from branch-22.02 to branch-21.12 November 17, 2021 17:33
cjnolet
cjnolet approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM.

@cjnolet
Copy link
Member

cjnolet commented Nov 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4d1d201 into rapidsai:branch-21.12 Nov 17, 2021
@achirkin achirkin deleted the fix-coalesced-access branch March 31, 2022 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review bug Something isn't working CMake cpp enhancement New feature or request non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@achirkin @dantegd @cjnolet