Update compiler flags for consistency with MOM5 - #27
dougiesquire wants to merge 5 commits into
Conversation
|
A couple of things slightly different than what was discussed:
|
|
This is ready for review together with ACCESS-NRI/MOM5#91 @harshula, @manodeep No answer changes with oneapi in ACCESS-OM2 or ACCESS-ESM1.6 (tested here and here) |
manodeep
left a comment
manodeep
left a comment
There was a problem hiding this comment.
Looks pretty good - I made some minor comments
|
|
||
| # Copied from MOM5/bin/mkmf.template.nci.gfortran | ||
| set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -fcray-pointer -fdefault-real-8 -ffree-line-length-none -fno-range-check -Waliasing -Wampersand -Warray-bounds -Wcharacter-truncation -Wconversion -Wline-truncation -Wintrinsics-std -Wsurprising -Wno-tabs -Wunderflow -Wunused-parameter -Wintrinsic-shadow -Wno-align-commons") | ||
| set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -fcray-pointer -ffree-line-length-none -fno-range-check -Waliasing -Wampersand -Warray-bounds -Wcharacter-truncation -Wconversion -Wline-truncation -Wintrinsics-std -Wsurprising -Wno-tabs -Wunderflow -Wunused-parameter -Wintrinsic-shadow -Wno-align-commons") |
There was a problem hiding this comment.
Both Intel/IntelLLVM branches have an -i4 embedded into the "always-on" flags but GNU does not - is that intentional?
There was a problem hiding this comment.
Also, do we need a big-endian-conversion flag?
There was a problem hiding this comment.
Both
Intel/IntelLLVMbranches have an-i4embedded into the "always-on" flags but GNU does not - is that intentional?
Yeah, as far as I know, there is no equivalent flag. 4-byte integers are gfortran's default. One can set -fdefault-integer-8, but there's no equivalent flag to "unset" it back to the default like there is for ifx/ifort.
There was a problem hiding this comment.
Also, do we need a
big-endian-conversionflag?
Yeah, probably a good idea. Added in 4a8501b
There was a problem hiding this comment.
Sorry - totally forgot to update here. I trawled through the gfortran docs and yes, there is no equivalent compiler flag for -i4
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}") | ||
| set(CMAKE_C_FLAGS_DEBUG "-O0 -g") | ||
| set(CMAKE_C_FLAGS_RELEASE "-O2") | ||
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}") | ||
| set(CMAKE_C_FLAGS_DEBUG "-O0 -g") | ||
| set(CMAKE_C_FLAGS_RELEASE "-O2") |
There was a problem hiding this comment.
I can't see any difference between these two changed lines? Are these whitespace-only changes?
There was a problem hiding this comment.
Yeah, I added CMAKE_C_FLAGS_RELWITHDEBINFO (below) and tried to align things for readability/consistency (though I see I missed a space 😅)
There was a problem hiding this comment.
Spacing is sorted in 4a8501b. If I don't adjust the whitespace for the existing sets they will be inconsistent with the new CMAKE_C_FLAGS_RELWITHDEBINFO set. But happy to do this if you prefer
There was a problem hiding this comment.
I am fine with whichever works for you. Was just making sure that I wasn't missing anything :D
|
Thanks @manodeep. Are you also planning on looking at the MOM5 counterpart PR? |
|
@dougiesquire LGTM (I had already approved) |
2 participants
This PR updates the compiler flags in the CMakeLists for consistency with what is used in MOM5 - see discussion here.
This is a temporary measure until we have a organisational recommendations for compiler flags. This change should not change any answers (will test)
To do:
build_typevariant in spack package (defaultRelWithDebInfo) - PR here