Update cmake config to support new NCEP libraries (imported targets) on Tier-2 platforms #177

climbfuji · 2020-07-29T20:11:43Z

Description

This PR is a follow-up and clean-up for #172.

updates the modulefiles and cmake configurations for Tier-2 platforms in order to work with the new NCEP libraries (imported targets)
cleans up cmake configurations (AVX2 flags, DEBUG and REPRO mode) and declares SIMDMULTIARCH flags as a real alternative to the AVX2 flags
removes old build targets cheyenne.intel-mpi and supermuc-phase2.intel
clean up of build.sh

Noting that for some platforms the responsibility for maintaining it may be handed off to somebody else and the directory layout/naming for the thirdparty and NCEP libraries may change as a result of it.

Issue(s) addressed

n/a

Testing

compiles and runs on macosx.gnu, gaea.intel, jet.intel, stampede.intel, cheyenne.gnu, cheyenne.intel
currently creating new baselines on cheyenne.gnu and cheyenne.intel using rt.sh
need to run regression tests on tier-1 platforms and cheyenne.gnu and cheyenne.intel once these PRs are at the top of the commit queue

Dependencies

Waiting on:
NOAA-EMC/fv3atm#150
NOAA-EMC/NEMS#74

…ake/Intel.cmake and cmake/GNU.cmake

…e review and testing

climbfuji · 2020-07-29T20:29:35Z

Ready from my side.

climbfuji · 2020-07-29T21:37:56Z

Regression testing on cheyenne.intel and cheyenne.gnu: first create new baselines (Cheyenne did not yet have a 2020724 baseline), then verify against it: all tests pass, regression test logs updated in PR.

rt_cheyenne_gnu_create.log
rt_cheyenne_gnu_verify.log
rt_cheyenne_intel_create.log
rt_cheyenne_intel_verify.log

climbfuji · 2020-07-29T22:02:28Z

Regression testing on hera.intel and hera.gnu against existing baseline: all tests pass, logs updated in the PR.

rt_hera_gnu.log
rt_hera_intel.log

climbfuji · 2020-07-30T12:56:23Z

Regression testing on orion.intel, wcoss_cray, wcoss_dell_p3 against existing baseline: all tests pass, logs updated in the PR.

aerorahul

There is incorrect use of defining compiler flags.
CMAKE_<lang>_FLAGS_<BUILDTYPE> should be defined for the appropriate CMAKE_BUILD_TYPE.
One should not "jam" those flags into the CMAKE_<lang>_FLAGS variable.
CMake knows what the BUILD TYPE is and will append the appropriate flags.

See:
https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html
https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html#variable:CMAKE_BUILD_TYPE

aerorahul · 2020-07-30T13:45:46Z

cmake/GNU.cmake

+    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS}")
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")


What the the purpose of this?
A=A?

I didn't write this code.

Understood.
Since there is cleaning e.g. AVX2 etc. and adding flags, it is appropriate to comment.

This is an addition. GREEN.

aerorahul · 2020-07-30T13:46:38Z

cmake/GNU.cmake

@@ -1,24 +1,23 @@

 set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -fcray-pointer -ffree-line-length-none -fno-range-check -fbacktrace")

+
 if(DEBUG)
    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -Wall -O0 -ggdb -fno-unsafe-math-optimizations -frounding-math -fsignaling-nans -ffpe-trap=invalid,zero,overflow -fbounds-check")


This is not the correct way of adding DEBUG flags.
Please add DEBUG flags in CMAKE variable for e.g.

set(CMAKE_Fortran_FLAGS_DEBUG "-Wall -O0 -ggdb -fno-unsafe-math-optimizations -frounding-math -fsignaling-nans -ffpe-trap=invalid,zero,overflow -fbounds-check" )

Cmake will automatically add these flags when the build_type is DEBUG.
See
https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html#variable:CMAKE_BUILD_TYPE

Please do this where DEBUG flags are set.

aerorahul · 2020-07-30T13:48:41Z

cmake/GNU.cmake

-        set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -O0 -ggdb")
-        set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0")
+    add_definitions(-DDEBUG)
+elseif(REPRO)


REPRO a CMAKE_BUILD_TYPE? If not, it should be defined as such.
Flags specific to REPRO should be defined as
CMAKE_<lang>_FLAGS_REPRO

aerorahul · 2020-07-30T13:49:19Z

cmake/GNU.cmake

    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS}")
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")


What is the purpose of this? A=A.

I didn't write this code.

aerorahul · 2020-07-30T13:49:59Z

cmake/Intel.cmake

@@ -18,29 +18,33 @@ if(REPRO)
    add_definitions(-DREPRO)
    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -qno-opt-dynamic-align")
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -qno-opt-dynamic-align")
+elseif(DEBUG)
+    add_definitions(-DDEBUG)


CMAKE_BUILD_TYPE is DEBUG.
Use appropriate CMAKE_<lang>_FLAGS_DEBUG

Please submit a follow-up PR to clean up the entire cmake build system. I am only making minimal changes to an existing cmake build system that already has DEBUG and REPRO defined. I don't think we want to make these changes now and rerun all the regression tests, but I may be mistaken.

This is new code added.
If you want to add something, adding it correctly won't impact older issues.
But, just building on older issues will make future clean-up harder.

If I make the changes as you suggested, I'll break the existing builds. You'll see why in a minute when I walk you throuh the legacy code.

aerorahul · 2020-07-30T14:03:15Z

cmake/Intel.cmake

    endif()
 endif()

 if(REPRO)
    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -O2 -debug minimal -fp-model consistent -qoverride-limits -g -traceback")
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O2 -debug minimal")
+elseif(DEBUG)
+    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -g -O0 -check -check noarg_temp_created -check nopointer -warn -warn noerrors -fp-stack-check -fstack-protector-all -fpe0 -debug -traceback -ftrapuv")


DEBUG flags are inappropriate in this CMAKE variable.

This is an addition. GREEN.

DusanJovic-NOAA · 2020-07-30T14:14:36Z

@aerorahul This needs to be cleaned up. For example DEBUG flags was used (instead of CMAKE_BUILD_TYPE) because that flag is used in GNU make build, so we wanted to maintain that during the transition and make maintenance of compile.sh scripts as similar as possible. There are many other redundant flags in both GNU and Intel configs (-qno-opt-dynamic-align for example). The original version of these files was just a rewrite of gnu make configuration files (ex. conf/configure.fv3.hera.intel).

We keep postponing this cleanup until we fully transition to cmake (ie. remove GNU build entirely).

climbfuji · 2020-07-30T14:17:23Z

@aerorahul This needs to be cleaned up. For example DEBUG flags was used (instead of CMAKE_BUILD_TYPE) because that flag is used in GNU make build, so we wanted to maintain that during the transition and make maintenance of compile.sh scripts as similar as possible. There are many other redundant flags in both GNU and Intel configs (-qno-opt-dynamic-align for example). The original version of these files was just a rewrite of gnu make configuration files (ex. conf/configure.fv3.hera.intel).

We keep postponing this cleanup until we fully transition to cmake (ie. remove GNU build entirely).

I'll have a quick chat with @aerorahul at 8.30 am MT to explain the origin for all this "build code". I'll invite you, too, in case you have time to attend.

climbfuji · 2020-07-30T17:16:04Z

@aerorahul Could you please approve the PR based on our earlier discussion, noting that we will fix the issues you raised as part of our grand cleanup work? Thanks ...

aerorahul

Following enlightening offline conversation with @climbfuji and @DusanJovic-NOAA and recognizing the details and need for keeping the flags until a later time when the GNU build is decommissioned, I approve this PR.

I am leaving the comments here for reference.

junwang-noaa

I did not closely follow the cmake development, it looks to me the changes shows no impact on the results. Maybe you can give some presentation on cmake build in ufs-weather-model once the cmake is able to replace the gnu build, thanks

climbfuji · 2020-07-30T18:14:23Z

Thanks, everyone!

MinsukJi-NOAA · 2020-08-03T17:26:45Z

@climbfuji, I see that in modulefiles/linux.gnu/fv3, library paths for NETCDF, ESMF, NCEPLIBS, and etc. have been removed. Where should I specify them? -- I am using this fv3 file in my github actions workflow currently.

climbfuji added 10 commits July 28, 2020 15:16

Remove cheyenne.intel-impi and supermuc_phase2.intel build targets

2cc56be

Clean up modulefiles/linux.gnu/fv3 and modulefiles/macosx.gnu/fv3

600b12e

Update stampede.intel configuration

709aa57

build.sh: change default suite, remove unused code

7cdebd2

Properly define SIMDMULTIARCH flags in CMakeLists.txt and clean up cm…

185ce4f

…ake/Intel.cmake and cmake/GNU.cmake

Remove unused AVX2 switch from cmake/configure_hera.gnu.cmake

d05674a

Update cmake build and modules for gaea.intel, jet.intel, macosx.gnu

3599ff4

Update cmake build and modules for cheyenne.intel and cheyenne.gnu

5e047ed

Update .gitmodules and submodule pointers for fv3atm and NEMS for cod…

6d5f3d1

…e review and testing

Fix typo in cmake/GNU.cmake

aae4add

climbfuji marked this pull request as ready for review July 29, 2020 20:12

This was referenced Jul 29, 2020

Remove cheyenne.intel-impi and supermuc_phase2.intel build targets NOAA-EMC/fv3atm#150

Merged

Remove cheyenne.intel-impi and supermuc_phase2.intel build targets NOAA-EMC/NEMS#74

Merged

climbfuji requested review from DusanJovic-NOAA, junwang-noaa and aerorahul July 29, 2020 20:15

Be consistent with the use of -ggdb

f73e30a

climbfuji added 2 commits July 29, 2020 14:40

Revert NEMS change to .gitmodules and update submodule pointer for NEMS

042fec6

Regression test logs for cheyenne.intel and cheyenne.gnu

e6ec5a4

Regression test logs for hera.gnu and hera.intel

f99190d

Regression test logs for orion.intel, wcoss_cray, wcoss_dell_p3

7b69993

Revert change to .gitmodules and update submodule pointer for fv3atm

9f0aa77

DusanJovic-NOAA approved these changes Jul 30, 2020

View reviewed changes

aerorahul requested changes Jul 30, 2020

View reviewed changes

aerorahul reviewed Jul 30, 2020

View reviewed changes

aerorahul approved these changes Jul 30, 2020

View reviewed changes

junwang-noaa approved these changes Jul 30, 2020

View reviewed changes

DusanJovic-NOAA merged commit 0d3b5ce into ufs-community:develop Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cmake config to support new NCEP libraries (imported targets) on Tier-2 platforms #177

Update cmake config to support new NCEP libraries (imported targets) on Tier-2 platforms #177

climbfuji commented Jul 29, 2020

climbfuji commented Jul 29, 2020

climbfuji commented Jul 29, 2020

climbfuji commented Jul 29, 2020

climbfuji commented Jul 30, 2020

aerorahul left a comment •

edited

Loading

aerorahul Jul 30, 2020

climbfuji Jul 30, 2020

aerorahul Jul 30, 2020

aerorahul Jul 30, 2020

aerorahul Jul 30, 2020

aerorahul Jul 30, 2020

aerorahul Jul 30, 2020

climbfuji Jul 30, 2020

aerorahul Jul 30, 2020

climbfuji Jul 30, 2020

aerorahul Jul 30, 2020

climbfuji Jul 30, 2020

aerorahul Jul 30, 2020

aerorahul Jul 30, 2020

DusanJovic-NOAA commented Jul 30, 2020

climbfuji commented Jul 30, 2020

climbfuji commented Jul 30, 2020

aerorahul left a comment

junwang-noaa left a comment

climbfuji commented Jul 30, 2020

MinsukJi-NOAA commented Aug 3, 2020

		set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS}")
		set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")

Update cmake config to support new NCEP libraries (imported targets) on Tier-2 platforms #177

Update cmake config to support new NCEP libraries (imported targets) on Tier-2 platforms #177

Conversation

climbfuji commented Jul 29, 2020

Description

Issue(s) addressed

Testing

Dependencies

climbfuji commented Jul 29, 2020

climbfuji commented Jul 29, 2020

climbfuji commented Jul 29, 2020

climbfuji commented Jul 30, 2020

aerorahul left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DusanJovic-NOAA commented Jul 30, 2020

climbfuji commented Jul 30, 2020

climbfuji commented Jul 30, 2020

aerorahul left a comment

Choose a reason for hiding this comment

junwang-noaa left a comment

Choose a reason for hiding this comment

climbfuji commented Jul 30, 2020

MinsukJi-NOAA commented Aug 3, 2020

aerorahul left a comment •

edited

Loading