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

Fix typo in ordering of fields in ZGMV #66

Merged

Conversation

samhatfield
Copy link
Collaborator

The calculation of the ordering of fields in zgmv is incorrect. First spotted by @marsdeno when he tried running with --uvders --scders --vordiv all enabled (I had never thought of testing all three at once!). The benchmark crashed with a "partially present on device" error referring to PGP3A.

UV-like fields and other 3D fields are both stored in the same array: zgmv. zgpuv and zgp3a are pointers pointing to different slices of this array, and these are subsequently passed to inv_trans and dir_trans:

zgpuv => zgmv(:,:,1:jend_vder_EW,:)
zgp3a => zgmv(:,:,jbegin_sc:jend_scder_EW,:)

(Here)

Without this fix (assuming nfld == 1):
jend_vder_EW = 6, jbegin_sc = 6, jend_scder_EW = 8: UV-like fields are arrange in entries 1:6, other 3D fields in entries 6:8. This is obviously wrong because the first entry of zgp3a is pointing to the same address as the last entry of the zgpuv.

The typo is here. jbegin_vder_EW should be jend_vder_EW. With this change the two pointer slices do not overlap.

Lessons learned:

  • We should add a test case with ALL benchmark arguments specified, but this still wouldn't have caught this bug because...
  • We should also find a way to execute the tests on a GPU-equipped machine. Is this possible with AC?

@wdeconinck
Copy link
Collaborator

Is this also a change that needs to be in develop?

@samhatfield
Copy link
Collaborator Author

Yes good point.

@wdeconinck wdeconinck merged commit 9c89429 into ecmwf-ifs:redgreengpu Mar 7, 2024
11 checks passed
@samhatfield samhatfield deleted the samhatfield/fix_uvder_bug branch June 5, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants