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

Use size_t for the number of shared memory bytes. #1046

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

bdice
Copy link
Member

@bdice bdice commented Jun 19, 2021

Description

Use size_t for the number of shared memory bytes for most (all?) kernels.

I did a find/replace for unsigned int shared_bytes so it's possible that I missed some kernels where the variable names are different. I also added const to some instances of shared_bytes, where appropriate.

Motivation and context

I was seeing compiler warnings when building HOOMD. These arose from the implicit casts of unsigned int to long unsigned int (the same as size_t on my system) for amounts of shared memory passed to CUDA kernels. Several places in the source code use explicit C-style casts (unsigned int)(value) or static_cast<unsigned int>(value) to avoid this kind of warning, but the actual data is probably best represented as size_t. Most of the places where this is used call sizeof(T), which returns size_t, and the HIP programming guide also says this value should be size_t when passed to the kernel run function.

This code path last changed in #809, which added the explicit casts in several places. It is likely that the new warnings I saw were from other PRs that were merged since then.

Sample warnings:

[356/525] Building CUDA object hoomd/hpmc/CMakeFiles/_hpmc.dir/kernel_gen_moves_ShapeSphere.cu.o
../hoomd/hpmc/IntegratorHPMCMonoGPUMoves.cuh: In instantiation of ‘void hpmc::gpu::hpmc_gen_moves(const hpmc::gpu::hpmc_args_t&, const typename Shape::param_type*) [with Shape = hpmc::ShapeSphere; typename Shape::param_type = hpmc::SphereParams]’:
hoomd/hpmc/kernel_gen_moves_ShapeSphere.cu:30:109:   required from here
../hoomd/hpmc/IntegratorHPMCMonoGPUMoves.cuh:298:43: warning: conversion from ‘long unsigned int’ to ‘unsigned int’ may change value [-Wconversion]
  298 |         unsigned int shared_bytes
      |                         ~~~~~~~~~         ^
../hoomd/hpmc/IntegratorHPMCMonoGPUMoves.cuh:354:43: warning: conversion from ‘long unsigned int’ to ‘unsigned int’ may change value [-Wconversion]
  354 |         unsigned int shared_bytes
      |                         ~~~~~~~~~         ^

...

[426/525] Building CUDA object hoomd/hpmc/CMakeFiles/_hpmc.dir/kernel_cluster_transform_ShapeSphere.cu.o
../hoomd/hpmc/UpdaterClustersGPU.cuh: In instantiation of ‘void hpmc::gpu::transform_particles(const hpmc::gpu::clusters_transform_args_t&, const typename Shape::param_type*) [with Shape = hpmc::ShapeSphere; typename Shape::param_type = hpmc::SphereParams]’:
hoomd/hpmc/kernel_cluster_transform_ShapeSphere.cu:30:130:   required from here
../hoomd/hpmc/UpdaterClustersGPU.cuh:726:60: warning: conversion from ‘long unsigned int’ to ‘unsigned int’ may change value [-Wconversion]
  726 |     unsigned int shared_bytes = sizeof(typename Shape::param_type) * args.num_types;
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

How has this been tested?

HOOMD builds without warnings now.

Compiler versions tested

gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
nvcc: release 11.0, V11.0.221

Checklist:

@bdice bdice requested review from a team, joaander and mariaward10 and removed request for a team June 19, 2021 01:34
@bdice
Copy link
Member Author

bdice commented Jun 19, 2021

I ran pre-commit run --all-files --hook-stage manual and it passed locally, but CI checks were failing. I found out that I needed a newer version of identify, because my installed version was older and didn't recognize .cuh files as targets for clang-format. It requires identify>=2.2.4 (the release after I added support for .cuh to identify) but I don't know where that requirement would go. It would need to belong to the environment containing pre-commit but I'm not sure how that would be enforceable on users' systems. Issue filed: pre-commit/pre-commit#1946

Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

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

LGTM, and if you can compile without warnings now, then it is likely you got all the places this needed to change. As for the pre-commit issue, if you think it is worth considering options to fix this, then please create an issue.

@b-butler b-butler added the validate Execute long running validation tests on pull requests label Jun 21, 2021
@b-butler
Copy link
Member

@mariaward10 do you know when you will have time to review this?

@b-butler b-butler merged commit 53fbf05 into master Jun 24, 2021
@b-butler b-butler deleted the fix/shared-mem-size_t branch June 24, 2021 15:27
Copy link

@mariaward10 mariaward10 left a comment

Choose a reason for hiding this comment

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

Earlier, I looked over all the changes made, and didn't find any issues with those. Seeing as the compiler warnings no longer occur, I believe this should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants