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

Add warning if GPU model grid Nx, Ny are not multiple of 16 #398

Merged
merged 2 commits into from
Sep 13, 2019

Conversation

ali-ramadhan
Copy link
Member

Add a warning if trying to create a GPU model with a grid where Nx or Ny are not a multiple of 16.

Some GPU kernels still use the hard-coded Tx = Ty = 16 for calculating thread-block layouts. See PR #308 for why.

Even if we were to get rid of the hard-coded Tx, Ty there will always be weird grid sizes like 17x33x47 that you can run no problem on a CPU, but might not be able to create a perfect thread-block layout for without having extra threads that do nothing, but extra threads can do bad stuff like apply a boundary condition twice if we're not careful.

Apologies to @beta-effect who was burned by this in the past.

@ali-ramadhan ali-ramadhan added bug 🐞 Even a perfect program still has bugs cleanup 🧹 Paying off technical debt labels Sep 11, 2019
Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

<3 for using grid.Nx, etc.

@glwagner
Copy link
Member

Hmm, actually do you want to move our hard-coded values of Tx, Ty to models.jl, and then reference them in the model constructor? Might be clearer.

@codecov
Copy link

codecov bot commented Sep 13, 2019

Codecov Report

Merging #398 into master will increase coverage by 1.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   64.62%   65.99%   +1.36%     
==========================================
  Files          23       23              
  Lines        1405     1532     +127     
==========================================
+ Hits          908     1011     +103     
- Misses        497      521      +24
Impacted Files Coverage Δ
src/models.jl 92.53% <100%> (-0.32%) ⬇️
src/time_steppers.jl 72.41% <0%> (-0.92%) ⬇️
src/boundary_conditions.jl 84.25% <0%> (+2.9%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 079cb53...5e26132. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 13, 2019

Codecov Report

Merging #398 into master will increase coverage by 5.82%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   64.62%   70.45%   +5.82%     
==========================================
  Files          23       23              
  Lines        1405     1682     +277     
==========================================
+ Hits          908     1185     +277     
  Misses        497      497
Impacted Files Coverage Δ
src/models.jl 91.3% <75%> (-1.56%) ⬇️
src/output_writers.jl 77.71% <0%> (+0.54%) ⬆️
src/time_steppers.jl 73.89% <0%> (+0.55%) ⬆️
src/turbulence_closures/TurbulenceClosures.jl 57.69% <0%> (+1.44%) ⬆️
src/boundary_conditions.jl 86.11% <0%> (+4.75%) ⬆️
src/Oceananigans.jl 90.9% <0%> (+7.57%) ⬆️
src/fields.jl 54.86% <0%> (+8.03%) ⬆️
src/utils.jl 64.44% <0%> (+14.44%) ⬆️
src/poisson_solvers.jl 76.41% <0%> (+36.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 079cb53...5e26132. Read the comment docs.

@ali-ramadhan ali-ramadhan merged commit 56ca04c into master Sep 13, 2019
@ali-ramadhan ali-ramadhan deleted the gpu16-warning branch September 13, 2019 14:57
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
Add warning if GPU model grid Nx, Ny are not multiple of 16

Former-commit-id: 56ca04c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Even a perfect program still has bugs cleanup 🧹 Paying off technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants