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

Refactor scalar reductions to use common execution policy #573

Merged

Conversation

jjwilke
Copy link
Contributor

@jjwilke jjwilke commented Sep 2, 2022

Following the design doc for improving code reuse, this implements all scalar reduction kernels through a single implementation.

  • Study verifies binary size is not increased
  • Study verifies performance is equivalent

@jjwilke jjwilke added the category:improvement PR introduces an improvement and will be classified as such in release notes label Sep 2, 2022
@magnatelee
Copy link
Contributor

@jjwilke please add a copyright header to the new files

@magnatelee
Copy link
Contributor

left two nitpick-y comments. otherwise looks good.

@jjwilke
Copy link
Contributor Author

jjwilke commented Sep 6, 2022

@magnatelee Should be ready to merge. There was an additional change required to unary_red_util.h you might want to glance at.

#define CUDA_FUNCTION
#endif

#endif // SRC_CUNUMERIC_EXECUTION_POLICY_EXECUTION_POLICY_HELPERS_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file. Weird that the clang auto-formatter didn't catch that. Does it look like clang-format is running when you make change to header files? It might be that the filename pattern is skipping header files, or maybe just our clang-format configuration isn't checking for missing newlines at end of files.

@magnatelee
Copy link
Contributor

@magnatelee Should be ready to merge. There was an additional change required to unary_red_util.h you might want to glance at.

Feel free to merge this once you remove the obsolete header file.

@jjwilke jjwilke merged commit e5f90b7 into nv-legate:branch-22.10 Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:improvement PR introduces an improvement and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants