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

Fixing openmp compile #1010

Merged
merged 5 commits into from
Oct 2, 2019

Conversation

nikizadehgfdl
Copy link
Contributor

  • openmp threads produce non-openmp stats
  • openmp 1 thread run has to be a test. Otherwise it would be safer not supporting openmp at all.

- Simple tests produce with threads
@codecov-io
Copy link

codecov-io commented Sep 30, 2019

Codecov Report

Merging #1010 into dev/gfdl will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           dev/gfdl    #1010   +/-   ##
=========================================
  Coverage     43.16%   43.16%           
=========================================
  Files           213      213           
  Lines         62352    62352           
=========================================
  Hits          26915    26915           
  Misses        35437    35437
Impacted Files Coverage Δ
...rc/parameterizations/vertical/MOM_diabatic_aux.F90 62.13% <ø> (ø) ⬆️
src/diagnostics/MOM_diagnostics.F90 87.38% <ø> (ø) ⬆️
src/tracer/MOM_tracer_advect.F90 69.03% <ø> (ø) ⬆️
...ameterizations/lateral/MOM_mixed_layer_restrat.F90 86.31% <ø> (ø) ⬆️
src/parameterizations/lateral/MOM_hor_visc.F90 62.67% <ø> (ø) ⬆️
src/core/MOM_barotropic.F90 71.92% <ø> (ø) ⬆️
src/core/MOM_PressureForce_blocked_AFV.F90 0% <ø> (ø) ⬆️
...arameterizations/lateral/MOM_thickness_diffuse.F90 63.7% <ø> (ø) ⬆️
...c/parameterizations/vertical/MOM_vert_friction.F90 72.12% <ø> (ø) ⬆️
...eterizations/lateral/MOM_lateral_mixing_coeffs.F90 59.61% <ø> (ø) ⬆️
... and 2 more

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 9ca5fb9...729a497. Read the comment docs.

@nikizadehgfdl
Copy link
Contributor Author

This fix is not complete. Dev/gfdl still has openmp issues. I don't know how I was able to compile this branch!!

- HOWEVER, Answers for 1 thread runs differ from answers with no openmp threads!!!
- Not acceptable
- One varible was set before OMP section and needs to be firstprivate.
- We have to check for consistency of answers between openmp and non-openmp builds.
@nikizadehgfdl
Copy link
Contributor Author

This branch fixes -openmp compilation issues as well as answer changes for threaded runs.
It closes issue #973
OMP threaded runs (ocean_nthreads>1) still underperform.

@nikizadehgfdl nikizadehgfdl reopened this Oct 1, 2019
!$OMP pen_TKE_2d,Temp_in,Salin_in,RivermixConst) &
!$OMP firstprivate(start,npts)
!$OMP firstprivate(start,npts,SurfPressure)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the answers mismatch between openmp and non-openmp execs.

@marshallward
Copy link
Collaborator

marshallward commented Oct 1, 2019

I just tested an OpenMP build and am also seeing errors in the MOM_geothermal.F90 code:

ftn -Duse_libMPI -Duse_netCDF -DSPMD -Duse_LARGEFILE  -I/opt/cray/pe/netcdf/4.4.0/GNU/5.1/include -fcray-pointer -fdefault-real-8 -fdefault-double-8 -Waliasing -ffree-line-length-none -fno-range-check -O2 -fno-expensive-optimizations -fopenmp -I /lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../deps/fms/build  -c -I/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../config_src/dynamic_symmetric -I/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../src/framework	/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../src/parameterizations/vertical/MOM_geothermal.F90
/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../src/parameterizations/vertical/MOM_geothermal.F90:158:60:

                   .or. CS%id_internal_heat_temp_tendency > 0
                                                            1
Error: Unexpected assignment statement at (1)
/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../src/parameterizations/vertical/MOM_geothermal.F90:161:60:

                   .or. CS%id_internal_heat_temp_tendency > 0
                                                            1
Error: Unexpected assignment statement at (1)
/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../src/parameterizations/vertical/MOM_geothermal.F90:163:65:

   if (CS%id_internal_heat_heat_tendency > 0) work_3d(:,:,:) = 0.0
                                                                 1
Error: Unexpected simple IF statement at (1)
/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../src/parameterizations/vertical/MOM_geothermal.F90:164:39:

   if (compute_h_old) h_old(:,:,:) = 0.0
                                       1
Error: Unexpected simple IF statement at (1)
/lustre/f2/dev/gfdl/Marshall.Ward/dev/mom6/.testing//../src/parameterizations/vertical/MOM_geothermal.F90:165:39:

   if (compute_T_old) T_old(:,:,:) = 0.0
                                       1
Error: Unexpected simple IF statement at (1)

Assuming it's not too much trouble, I think we should probably add this to the test suite, if only to check that it builds.

Edit: Niki's patch will fix this issue (along with others)

@marshallward
Copy link
Collaborator

Gaea regression tests: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/pipelines/9074

@marshallward
Copy link
Collaborator

Gaea regressions have passed.

I can also build this internally although I am seeing some differences in answers for some of the TCs in the new test suite. But I will look into those in a different PR.

@marshallward marshallward merged commit d38c0dc into mom-ocean:dev/gfdl Oct 2, 2019
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