Skip to content
This repository has been archived by the owner on Oct 23, 2020. It is now read-only.

New compiler options and updates of top-level makefile #1087

Merged
merged 5 commits into from
Nov 10, 2016

Conversation

climbfuji
Copy link
Contributor

New compiler options ifort-mic, ifort-scorep and gfortran-clang are added. The legacy openmp flags for the intel compiler are updated, and GPTL support is added for every compiler option.

Further, this version also supports netCDF installations in which the netCDF-fortran libraries reside in a different place than the c libraries (common practice at several HPC centres). The environment variable used for this is usually NETCDFF.

@mgduda
Copy link
Contributor

mgduda commented Oct 6, 2016

@climbfuji What do you think about splitting commit 5478728 into one commit that corrects the -openmp flag and a separate commit to add new build targets. These two things seem independent, and it would allow us to more easily request changes to one or the other.

@climbfuji
Copy link
Contributor Author

@mgduda ok with me, too - you want me to do that?

…compiler options ifort-mic, ifort-scorep, gfortran-clang
…lled in a different place than the C library (env. variable NETCDFF)
@climbfuji climbfuji force-pushed the framework/top-level-Makefile branch from 03d4f5d to 6a386b4 Compare November 2, 2016 08:04
@climbfuji
Copy link
Contributor Author

climbfuji commented Nov 2, 2016

@matthewhoffman - Hi Michael & Matthew, I cleaned up the commit using an interactive rebase as suggested by Michael. I also did tests with the atmosphere/init_atmosphere/ocean cores.

@mgduda
Copy link
Contributor

mgduda commented Nov 6, 2016

I've done some testing of the logic to detect OpenMP compiler support, and everything seems to be working there so far. When I don't build with OPENMP=true, nothing new happens, and when I do build with OPENMP=true, the build goes through the logic of ensuring that the compiler can compile a test program with OpenMP flags. Since all compilers that I have access to do support OpenMP, I just tried compiling with a compiler that I don't have, and that causes the build to stop, which is good.

The only issue that I can see is with compilers that continue with only warnings when unrecognized options are specified. For example, with the Intel compilers, if I change the (correct) OpenMP flag from -qopenmp to -qomp, I get these warnings during the compiler test:

Testing compiler for OpenMP support
icc: command line warning #10006: ignoring unknown option '-qomp'
icc: command line warning #10006: ignoring unknown option '-qomp'
icpc: command line warning #10006: ignoring unknown option '-qomp'
ifort: command line warning #10006: ignoring unknown option '-qomp'
ifort: command line warning #10006: ignoring unknown option '-qomp'

but the build continues.

@climbfuji what if we included some actual OpenMP calls in the conftest programs, e.g.,

        @echo "int main() { int n = omp_get_num_threads(); return 0; }" > conftest.c; $(SCC) $(CFLAGS) -o conftest.out conftest.c || \
                (echo "$(SCC) does not support OpenMP - see INSTALL in top-level directory for more information"; rm -fr conftest.*; exit 1)

to ensure that OpenMP is actually working, and that the compiler isn't just ignoring OpenMP flags?

@mgduda
Copy link
Contributor

mgduda commented Nov 6, 2016

@climbfuji is there a particular reason for going with -O2 rather than -O3 in the ifort-mic target?

@climbfuji
Copy link
Contributor Author

@mgduda - yes, with -O3 the code did not compile on the Intel MIC Knights Corner at SuperMUC. Maybe depends on the exact version of the compiler and software stack on the card. As we said, the mic option won't be used by many (and those using them will probably be fairly experienced, given the current "user-friendlyness" of the MICs) so they will probably play with the switches after having the code compiled and tested successfully?

@climbfuji
Copy link
Contributor Author

@mgduda what do you think about tailoring the intel-mic comiler options to the Knights Landing architecture, leaving aside the Knights Corner? Compilation is substantially different and given all the hassle that people have with cross-compiling for the KNC and the poor performance, the KNL settings would be more useful. One could state that in the INSTALL notes that come along with this PR.

@mgduda
Copy link
Contributor

mgduda commented Nov 7, 2016

@climbfuji I like the idea of tuning the intel-mic option for the newer architecture. I've been using the following flags in my KNL testing: -xMIC-AVX512 -align array64byte -O3. I'm not sure how -xMIC-AVX512 differs from -mmic, but I think the -align array64byte -O3 flags are worth considering in the default options for this target.

Also, what do you think about naming the target intel-phi? This seems to be more in line with Intel's current naming for KNL parts, I think.

@climbfuji
Copy link
Contributor Author

@mgduda sounds good to me, I use the same options for knl. Let's briefly discuss after the telcon? Then I can explain the difference to -mmic and so on.

@climbfuji
Copy link
Contributor Author

climbfuji commented Nov 8, 2016

@mgduda - final commit from my side:

  • The ifort-mic option is now called ifort-phi. The conservative compiler options worked for me on the Knights Landing test system in Juelich. Using "-xMIC-AVX512" caused the model runs to abort for the small test cases I used (forrtl: severe (154): array index out of bounds), therefore I removed it (and also the "-align array64byte") from the default options to be on the safe side (see INSTALL file).
  • I tested the new Makefile, in particular the openmp compiler test, on all new build targets and for all model cores. I also repeated the tests with the new Makefile on the standard intel and bluegene targets.
  • The newly added INSTALL file gives additional information on some of the build targets and otherwise refers to the website.
  • Added "-g" to the non-debug build for ifort-scorep to enable profiling of code in production mode (negligible overhead).
  • I removed the -fpe0 option from the intel CFLAGS and CXXFLAGS, since this is an ifort option only.
  • I ran tests on the new build targets for the atmosphere core (ifort-phi) or the atmosphere+init_atmosphere+ocean core (ifort-scorep, gfortran-clang).

…ly. Adapted Intel Xeon Phi build target to Knights Landing architecture. Added "-g" option to non-debug build for scorep (otherwise can't profile code in production). Additional information on build targets in INSTALL file.
@climbfuji climbfuji force-pushed the framework/top-level-Makefile branch from b56fd20 to 42e8822 Compare November 8, 2016 05:51
@mgduda
Copy link
Contributor

mgduda commented Nov 8, 2016

@climbfuji Without any special compiler flags, the ifort-phi option is really only different from the ifort option in that the former uses the Intel MPI wrappers mpiifort, etc., rather than the more generic mpif90, etc. I do think this diminishes the value of this build target somewhat. What do you think?

@climbfuji
Copy link
Contributor Author

@mgduda whichever works for you. once could then say in INSTALL that phi users should use the standard ifort option but may experiment with the settings "...." (already listed in INSTALL) - given the short time it is best that you decide and implement the changes in the way it seems best for you.

@mgduda
Copy link
Contributor

mgduda commented Nov 9, 2016

@climbfuji That sounds good to me regarding the Phi and INSTALL files. I think for now we'll just remove the ifort-phi option.

@matthewhoffman as a somewhat urgent request, would you be able to look through this PR today and see whether you have any other concerns with merging this before the creation of the release-v5.0 branch?

Until further testing can be done, we remove the 'ifort-phi' build
target from the top-level Makefile and from the INSTALL file.
@mgduda
Copy link
Contributor

mgduda commented Nov 9, 2016

I just pushed a commit to remove the ifort-phi target. Once we've had some time to do broader testing on Xeon Phi, I think we can update the INSTALL file in the release branch. Otherwise, everything looks good to me.

@climbfuji
Copy link
Contributor Author

Looks good - go ahead, please! Release 5.0 on a special day it seems.

@mgduda
Copy link
Contributor

mgduda commented Nov 10, 2016

I haven't heard any objections, so I'll go ahead and merge this.

@mgduda mgduda merged commit bdbc919 into MPAS-Dev:develop Nov 10, 2016
@mgduda mgduda deleted the framework/top-level-Makefile branch November 10, 2016 23:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants