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.

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

Move contractions tiling logic outside of Contractions_NT #837

Merged

Conversation

ahendriksen
Copy link
Contributor

@ahendriksen ahendriksen commented Sep 22, 2022

The main functionality of Contractions_NT involves loading tiles of data into shared memory to enable fast GEMM-like kernels. In practice, this requires keeping track of tiles of data (2D submatrices of a bigger matrix) and distributing the data in the tiles over shared memory and registers of thread in a thread block.

Currently, Contractions_NT performs indexing logic for both:

  1. The distribution of data in a tile over registers and shared memory;
  2. Looping over tiles of data in a 2D matrix.

In this PR, we move functionality 2 out of Contractions_NT. Moving over the tiles of data and keeping track of the grid stride loop is now the responsibility of the calling code.

Splitting these responsibilities is helpful when non-trivial tiling logic is required, as in the upcoming sparseL2NN functionality.

Note: This PR also cleans up one unfortunate wart in the current implementation. Depending on which of the two overloaded constructors was called, the tiling logic was transposed leading to extremely difficult to track down bugs.

@ahendriksen ahendriksen requested a review from a team as a code owner September 22, 2022 12:54
@github-actions github-actions bot added the cpp label Sep 22, 2022
@ahendriksen ahendriksen added enhancement New feature or request 2 - In Progress Currenty a work in progress labels Sep 22, 2022
@ahendriksen ahendriksen force-pushed the wip-move-contractions-tiling-logic branch from 9029eed to 7222e23 Compare January 11, 2023 13:37
@ahendriksen ahendriksen changed the base branch from branch-22.10 to branch-23.02 January 11, 2023 13:38
@ahendriksen ahendriksen requested review from a team as code owners January 11, 2023 13:38
@ahendriksen ahendriksen added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 11, 2023
@ajschmidt8
Copy link
Member

Please use GitHub's Draft PR feature instead of WIP tags in the future. Draft PRs have the benefit of preventing notifications to codeowners until PRs are marked Ready for Review, which helps cut down on excessive notifications for PRs that are still being worked on. CI will still run on Draft PRs.

Some useful information about Draft PRs:

@ajschmidt8 ajschmidt8 marked this pull request as draft January 11, 2023 14:25
@ahendriksen ahendriksen requested review from tfeher and removed request for a team January 11, 2023 14:31
@ahendriksen ahendriksen changed the title [WIP] Move contractions tiling logic outside of Contractions_NT Move contractions tiling logic outside of Contractions_NT Jan 11, 2023
@ahendriksen ahendriksen marked this pull request as ready for review January 11, 2023 14:41
@ahendriksen
Copy link
Contributor Author

ahendriksen commented Jan 11, 2023

@tfeher @cjnolet : I have rebased the branch onto the latest 23.02 branch. It would be nice to get this PR merged. Both for sparseL2NN and hopper-based pairwise distance kernels.

@ahendriksen ahendriksen force-pushed the wip-move-contractions-tiling-logic branch from 7222e23 to d6a4587 Compare January 12, 2023 12:48
This results in subtle issues with non-square KernelPolicy, as found in
fusedL2KNN.
@ahendriksen ahendriksen force-pushed the wip-move-contractions-tiling-logic branch from 7b93722 to e6976c5 Compare January 24, 2023 09:09
@codecov-commenter
Copy link

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (4947dc8) compared to base (7c12b1e).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-23.02     #837   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cjnolet cjnolet added 5 - Ready to Merge and removed 2 - In Progress Currenty a work in progress labels Jan 25, 2023
@cjnolet cjnolet mentioned this pull request Jan 26, 2023
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.

Holding off on merging right away so we can investigate the CI timeouts.

@github-actions github-actions bot added the CMake label Jan 26, 2023
@cjnolet
Copy link
Member

cjnolet commented Jan 27, 2023

/merge

@rapids-bot rapids-bot bot merged commit c58d00a into rapidsai:branch-23.02 Jan 27, 2023
cjnolet added a commit to cjnolet/raft that referenced this pull request Feb 2, 2023
@ahendriksen ahendriksen deleted the wip-move-contractions-tiling-logic branch March 17, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge CMake cpp enhancement New feature or request gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

5 participants