ufs-community / ufs-mrweather-app

UFS Medium-Range Weather Application
Other
23 stars 23 forks source link

gfortran-10 flags in CIME #215

Closed climbfuji closed 4 years ago

climbfuji commented 4 years ago

For supporting gfortran-10, we had to add the following code in ufs-weather-model's cmake/GNU.cmake.

# For gfortran-10+ backward compatibility
if(${CMAKE_Fortran_COMPILER_ID} STREQUAL "GNU" AND ${CMAKE_Fortran_COMPILER_VERSION} VERSION_GREATER 9.9)
  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -w -fallow-argument-mismatch -fallow-invalid-boz")
endif()

I don't see these flags anywhere in the CIME config:

heinzeller-lt:ufs-mrweather-app-release-public-v1 dom.heinzeller$ grep -Rie 'fallow-argument-mismatch' *
src/post/sorc/ncep_post.fd/CMakeLists.txt:  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -w -fallow-argument-mismatch")
src/model/cmake/GNU.cmake:  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -w -fallow-argument-mismatch -fallow-invalid-boz")
src/model/FV3/ccpp/CMakeLists.txt:    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -w -fallow-argument-mismatch -fallow-invalid-boz")

The trouble is that we can't just add them to the GNU flags as default, because these flags are not recognized by gfortran 9.x and earlier.

Question: I believe we need to add them somewhere in the CIME config, and only if gfortran-10+ is used (auto-detect!), but I may be wrong. And certainly more than happy to be corrected.

climbfuji commented 4 years ago

Turns out we need those flags:

Building atm with output to /Users/dom/scratch/ufs-mrweather-app/UFS_SCRATCH/ufs-mrweather-app-workflow.c96.llvm/bld/atm.bldlog.201001-085501
Error: BOZ literal constant at (1) is neither a data-stmt-constant nor an actual argument to INT, REAL, DBLE, or CMPLX intrinsic function [see '-fno-allow-invalid-boz']

Error: Symbol 'domain_id_base' at (1) has no IMPLICIT type

Error: BOZ literal constant at (1) is neither a data-stmt-constant nor an actual argument to INT, REAL, DBLE, or CMPLX intrinsic function [see '-fno-allow-invalid-boz']

Error: Symbol 'domain_id_base' at (1) has no IMPLICIT type

ufsatm built in 12.988538 seconds
ERROR: BUILD FAIL: ufsatm.buildlib failed, cat /Users/dom/scratch/ufs-mrweather-app/UFS_SCRATCH/ufs-mrweather-app-workflow.c96.llvm/bld/atm.bldlog.201001-085501

@uturuncoglu @jedwards4b is there a way to do this in CIME? We need to detect that it is gfortran-10+ and then add the flags as shown in the description of this PR. We can't add them in general, because gfortran-9 and earlier don't recognize them and throw an error. Thank you ...

rsdunlapiv commented 4 years ago

@climbfuji another option here is to list gfortran-10 as an unsupported compiler

climbfuji commented 4 years ago

Of course, but we explicitly added the compatibility flags to NCEPLIBS-external, NCEPLIBS and ufs-weather-model in order to support it.

On Oct 1, 2020, at 9:02 AM, Rocky Dunlap notifications@github.com wrote:

@climbfuji https://github.com/climbfuji another option here is to list gfortran-10 as an unsupported compiler

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ufs-community/ufs-mrweather-app/issues/215#issuecomment-702197359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5C2RONYKLIQEQW2XO4JYTSISKZXANCNFSM4R72LXYA.

jedwards4b commented 4 years ago

@climbfuji we don't have a way to distinguish between compiler versions unless we define a new compiler, we currently define compiler "gnu" we could introduce a new compiler "gnu10".

climbfuji commented 4 years ago

@climbfuji we don't have a way to distinguish between compiler versions unless we define a new compiler, we currently define compiler "gnu" we could introduce a new compiler "gnu10".

Argh, that would be ugly. Maybe we can have a workaround for this release (i.e. tell the users to patch the CIME config_compilers.xml file with a patch file that we provide). Let me have a look, maybe there is a better way.

climbfuji commented 4 years ago

@climbfuji we don't have a way to distinguish between compiler versions unless we define a new compiler, we currently define compiler "gnu" we could introduce a new compiler "gnu10".

Argh, that would be ugly. Maybe we can have a workaround for this release (i.e. tell the users to patch the CIME config_compilers.xml file with a patch file that we provide). Let me have a look, maybe there is a better way.

Hmm. I thought I could be smart and put cmake code in a cmake configure file, but it doesn't work. I added the following to src/model/FV3/cime/cime_config/configure_cime.cmake:

if(${CMAKE_Fortran_COMPILER_ID} STREQUAL "GNU" AND ${CMAKE_Fortran_COMPILER_VERSION} VERSION_GREATER 9.9)
  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -w -fallow-argument-mismatch -fallow-invalid-boz")
endif()

but neither CMAKE_Fortran_COMPILER_ID nor CMAKE_Fortran_COMPILER_VERSION are known to cmake.

ligiabernardet commented 4 years ago

I would be glad to add to the documentation to explain how to patch the CIME config_compilers.xml file

jedwards4b commented 4 years ago

Does this line https://github.com/ufs-community/ufs-weather-model/blob/release/public-v2/CMakeLists.txt#L63 not print the compiler version?

climbfuji commented 4 years ago

Does this line https://github.com/ufs-community/ufs-weather-model/blob/release/public-v2/CMakeLists.txt#L63 not print the compiler version?

This one does, and this was also my next attempt - insert the code block in the top-level CMakeLists.txt (where it doesn't really belong, but so what). It seems that when src/model/FV3/cime/cime_config/configure_cime.cmake is parsed for the first time it's not the usual cmake call, but something else. I can't describe it, because I don't understand it.

jedwards4b commented 4 years ago

I think maybe the lines at 58-60 are what allows those variables to be resolved, can you try moving them to be above line 33?

climbfuji commented 4 years ago

It works if I make the following change to the top-level CMakeLists.txt in the ufs-weather-model:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index b35eb44..c06d686 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -21,6 +21,11 @@ find_package(ESMF REQUIRED)

 include(${PROJECT_SOURCE_DIR}/cmake/configure_${CMAKE_Platform}.cmake)

+if(${CMAKE_Fortran_COMPILER_ID} STREQUAL "GNU" AND ${CMAKE_Fortran_COMPILER_VERSION} VERSION_GREATER 9.9)
+  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -w -fallow-argument-mismatch -fallow-invalid-boz")
+  message(STATUS "Adjusting CMAKE_Fortran_FLAGS for gfortran-10 compatibility: '${CMAKE_Fortran_FLAGS}'")
+endif()
+
 if(NOT DEFINED PHYS)
   set(PHYS gfs)
 endif()

I don't think this code should be committed, because it's totally against the structure of the build system. It should really go into configure_cime.cmake, I don't understand why this is parsed differently (twice maybe, and it's the first time that fails because it is not the actual cmake build call?)

jedwards4b commented 4 years ago

I don't understand your comments about this file being parsed differently or twice - what leads you to that conclusion? This file is included at line 37 just as your platform specific files are.

climbfuji commented 4 years ago

Hmm I am confused by your line numbers, hopefully I am looking at the same files. I see the following in the top-level CMakeLists.txt:

set(CMAKE_C_COMPILER $ENV{CMAKE_C_COMPILER})
set(CMAKE_CXX_COMPILER $ENV{CMAKE_CXX_COMPILER})
set(CMAKE_Fortran_COMPILER $ENV{CMAKE_Fortran_COMPILER})
set(CMAKE_Platform $ENV{CMAKE_Platform})

project(NEMSfv3gfs C CXX Fortran)

list(APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake)
find_package(MPI REQUIRED)
find_package(ESMF REQUIRED)

include(${PROJECT_SOURCE_DIR}/cmake/configure_${CMAKE_Platform}.cmake)

This is line 22. Are you looking at the release/public-v1 code?

This suggests that CMAKE_Fortran_COMPILER etc is defined before configure_cime.cmake is imported.

climbfuji commented 4 years ago

I understand it now. The culprit is right here:

Calling cmake  -DCOMPILER=gnu  -DMPILIB=mpich  -DDEBUG=FALSE  -DCCPP=ON -DSUITES=FV3_GFS_v15p2,FV3_GFS_v15p2_no_nsst  -DMPI=ON  -DSTATIC=ON  -DOPENMP=OFF  -DNETCDF_DIR=/usr/local/ufs-release-v1.1.0/llvm  -C /Users/dom/scratch/ufs-mrweather-app/UFS_SCRATCH/ufs-mrweather-app-workflow.c96.llvm/bld/atm/obj/cmake/configure_cime.cmake /Users/dom/scratch/ufs-mrweather-app/UFS_SCRATCH/ufs-mrweather-app-workflow.c96.llvm/bld/atm/obj

The -C means, according to the cmake help, means:

  -C <initial-cache>           = Pre-load a script to populate the cache.

So this file is parsed before cmake ever looks at the top-level CMakeLists.txt (which then parses it again). Why is this done?

jedwards4b commented 4 years ago

Probably a stab in the dark in early development, try removing it.

jedwards4b commented 4 years ago

I was looking at v2. Do you plan to backport gnu-10 to v1?

climbfuji commented 4 years ago

gfortran-10 support is already in v1.1 of NCEPLIBS-external, NCEPLIBS and ufs-weather-model. Just not yet in CIME. But I think removing the -C line in buildlib solves the problem.

climbfuji commented 4 years ago

Remember v2 is for the December UFS Short-Range Weather App v1.0 release.

v1.1 (using the release/public-v1 branches for development, tags ufs-v1.1.0) is for the MRW App release v1.1.

climbfuji commented 4 years ago

Ok, it builds fine when I remove the -C line and put the gfortran-10 detection flags in src/model/FV3/cime/cime_config/configure_cime.cmake. I'll submit PRs for wherever it's necessary. Once we fixed this and the still open issue to run the regression tests on Stampede (yet another open issue), we should be able to go to the final tests before the release.

uturuncoglu commented 4 years ago

@climbfuji do you want to close this issue?

climbfuji commented 4 years ago

Closed via https://github.com/ESCOMP/FV3GFS_interface/pull/15 - thanks @uturuncoglu and @jedwards4b for your help.