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.

Sign up for GitHub

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

Jump to bottom

Fix OpenACC deletes for topographic wave drag #6471

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Fix OpenACC deletes for topographic wave drag #6471

merged 1 commit into from
Jun 27, 2024

Conversation

xylar
Copy link
Contributor

@xylar xylar commented Jun 13, 2024

Partially addresses #6470

The errors were introduced in #6310, when variables were introduced and renamed, including in the OpenACC create directives but corresponding changes were incomplete for the OpenACC directives for deleting them.

@xylar xylar added mpas-ocean bug fix PR BFB PR leaves answers BFB labels Jun 13, 2024
@xylar xylar requested a review from sbrus89 June 13, 2024 09:54
@xylar xylar mentioned this pull request Jun 13, 2024
Closed
@sbrus89
Copy link
Contributor

sbrus89 commented Jun 13, 2024

@xylar, just an FYI: in testing this with SMS_Ld1.T62_oEC60to30v3.CMPASO-NYF.pm-gpu_nvidiagpu I'm seeing this other build error:

NVFORTRAN-S-0155-DO loop expected after loop collapse() (/pscratch/sd/s/sbrus/e3sm_scratch/pm-gpu/SMS_Ld1.T62_oEC60to30v3.CMPASO-NYF.pm-gpu_nvidiagpu.20240613_121539_t9vbh9/bld/cmake-bld/core_ocean/shared/mpas_ocn_vmix.f90: 1461)
  0 inform,   0 warnings,   1 severes, 0 fatal for ocn_vmix_implicit

I'll look into it and push any changes to your branch.

@philipwjones
Copy link
Contributor

@sbrus89 Don't see any obvious problems with the loop its pointing too, but you might try removing the inner loop vector constructs - we've found those to be a bit flaky and the compiler generally vectorizes the loop anyway.

philipwjones
philipwjones approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

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

Leaving on vacation so can't review any future mods, but the changes here look fine. Will approve these changes and assume others will find any remaining issues.

mark-petersen
mark-petersen approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

@xylar thanks for catching this. This clearly matches the create lines here:
https://github.com/E3SM-Project/E3SM/pull/6310/files#diff-72984751a085d56765a30726c289582fe4f2e152d9e3b78dc06a405b19406879R738

I test compiled with gnu and intel, but don't have a workflow to test OpenACC. Thanks @sbrus89 for testing above. That looks like an unrelated error, so I think we can merge this one and then deal with that separately.

@sbrus89
Copy link
Contributor

sbrus89 commented Jun 20, 2024 •
edited
Loading

I found the issue with mpas_ocn_vmix.f90. The line numbers were misleading because they refer to the bld/cmake-bld code where the #ifdef directories are stripped out. Right now, the commit (8edf0a5) is for this fix is on my branch for #6472, but should probably move to a separate PR. I also found what I believe is a missing !$omp parallel region in mpas_ocn_vmix.f90. I was able to successfully compile SMS_Ld1.T62_oEC60to30v3.CMPASO-NYF.pm-gpu_nvidiagpu with the combination of #6471 (this PR) and #6472.

@xylar
Copy link
Contributor Author

xylar commented Jun 21, 2024

@sbrus89, I'm okay with keeping the vmix fixes in #6472 rather than further separating them out. Could you approve this if you're happy with it?

sbrus89
sbrus89 approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

Approved based on testing above. Thanks @xylar!

@sbrus89 sbrus89 mentioned this pull request Jun 21, 2024
Merged
jonbob added a commit that referenced this pull request Jun 26, 2024
@jonbob
Merge branch 'xylar/ocn/fix-topographic-wave-drag-openacc' into next …
6872355 
…(PR #6471)

Fix OpenACC deletes for topographic wave drag

Partially addresses #6470

The errors were introduced in #6310, when variables were introduced and
renamed, including in the OpenACC create directives but corresponding
changes were incomplete for the OpenACC directives for deleting them.

[BFB]
@jonbob
Copy link
Contributor

jonbob commented Jun 26, 2024

Passes:

  • ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel
  • SMS_D_Ld1.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF.chrysalis_intel.mpaso-jra_1958

merged to next

@jonbob jonbob merged commit f87bb7e into E3SM-Project:master Jun 27, 2024
13 checks passed
@jonbob
Copy link
Contributor

jonbob commented Jun 27, 2024

merged to master

xylar added a commit to xylar/compass that referenced this pull request Jul 3, 2024
@xylar
Update the E3SM-Project submodule
21b16e7 
This merge updates the E3SM-Project submodule from [31f771c](https://github.com/E3SM-Project/E3SM/tree/31f771c) to [c7d7998](https://github.com/E3SM-Project/E3SM/tree/c7d7998).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#6472
- [ ]  (ocn) E3SM-Project/E3SM#6471
- [ ]  (ocn) E3SM-Project/E3SM#6481
xylar added a commit to xylar/compass that referenced this pull request Jul 4, 2024
@xylar
Update the E3SM-Project submodule
9604e30 
This merge updates the E3SM-Project submodule from [31f771c](https://github.com/E3SM-Project/E3SM/tree/31f771c) to [c7d7998](https://github.com/E3SM-Project/E3SM/tree/c7d7998).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#6472
- [ ]  (ocn) E3SM-Project/E3SM#6471
- [ ]  (ocn) E3SM-Project/E3SM#6481
@xylar xylar mentioned this pull request Jul 4, 2024
Merged
3 tasks
xylar added a commit to xylar/compass that referenced this pull request Jul 4, 2024
@xylar
Update the E3SM-Project submodule
25fb79c 
This merge updates the E3SM-Project submodule from [31f771c](https://github.com/E3SM-Project/E3SM/tree/31f771c) to [c7d7998](https://github.com/E3SM-Project/E3SM/tree/c7d7998).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#6472
- [ ]  (ocn) E3SM-Project/E3SM#6471
- [ ]  (ocn) E3SM-Project/E3SM#6481
xylar added a commit to xylar/compass that referenced this pull request Jul 4, 2024
@xylar
Update the E3SM-Project submodule
0386931 
This merge updates the E3SM-Project submodule from [31f771c](https://github.com/E3SM-Project/E3SM/tree/31f771c) to [c7d7998](https://github.com/E3SM-Project/E3SM/tree/c7d7998).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#6472
- [ ]  (ocn) E3SM-Project/E3SM#6471
- [ ]  (ocn) E3SM-Project/E3SM#6481
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipwjones philipwjones philipwjones approved these changes

@mark-petersen mark-petersen mark-petersen approved these changes

@sbrus89 sbrus89 sbrus89 approved these changes

Assignees

@jonbob jonbob

Labels
BFB PR leaves answers BFB bug fix PR mpas-ocean
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xylar @sbrus89 @philipwjones @jonbob @mark-petersen