-
Notifications
You must be signed in to change notification settings - Fork 666
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
Resolves building issues (WRF/WRF-chem) with Intel compilers (ifx/icx) #1942
Conversation
arch/configure.defaults
Outdated
SCC = icx # icc | ||
CCOMP = icx # icc | ||
DM_FC = mpiifx #mpif90 -f90=$(SFC) | ||
DM_CC = mpiicx #mpicc -cc=$(SCC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Well, what should I do with this PR for the next step? Should I convert it as a draft and make a new commit and submit this PR again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahheo Please continue to modify this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiwangncar I have made this modification following your suggestion: restoring the changed option and adding another option as PR #1893 , by another commit 7361882. Well, the commit still failed in WRF regression test, and I was not able to check the details. Sorry that this is my first time making a PR, and I am still learning the procedures to make things work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahheo The failure of the regression tests is not your fault. We currently have some trouble running the tests on the Cloud. We are working to resolve the issues.
@ahheo Can we assume that your changes are limited in chem/KPP/ directory, while the rest is identical to that in PR#1823? |
Well, you might, in principle, assume this. But, to make it precisely, it is worth mentioning that I also made some modifications dealing with the following compilation warnings: |
@ahheo It would be helpful if you identify the routines you modified in addition to those modified in PR#1823, and explain why you need to modify them. |
The regression test results:
|
@ahheo It would be helpful if you identify the routines you modified in addition to those modified in PR#1823, and explain why you need to modify them. |
@ahheo I tried to use Intel oneAPI to compile WRF-Chem + KPP, it failed. It worked when compiling KPP only. Have you tried to compile WRF-Chem and KPP? I also verified that your modification still works for compiling WRF along. |
@weiwangncar, yes, I compiled with WRF_CHEM=1 together with WRF_KPP=1 using ifx / icx / mpiifx / mpiicx successfully, (and also successfully using ifx / icx / mpif90 -fc ifx / mpicc -cc icx). It would be nice if you can provide the compile log. |
@ahheo Here are the steps I took to compile your branch:
Here is the log file. |
@weiwangncar the compile problem is due to the failure in finding libfl.a. I found the following lines in your make log:
|
@ahheo Thanks for pointing it out. True, /usr/lib64 does not have libfl.a, it has /usr/libfl.so. Let me find out more about this lib. |
@ahheo Replacing libfl.a by libfl.so allowed the code to compile. |
@ahheo You probably received an email as well. If you have not, the regression tests have failed. Mostly because it failed to compile for all. |
@weiwangncar Hope this commit 26964a0 works fine. |
@ahheo The last change did not pass the regression tests. I have moved this to the develop branch to give both you and us more time to review and test the change. |
@weiwangncar Absolutely OK. It would be nice if you can tell more information why the regression tests failed. |
@ahheo The problem is that no output was sent back. This can indicate serious problems. Have you succeeded in compiling the updated code on your local computer? |
@weiwangncar Yes, I successfully compiled the updated code on my own machine. So, something goes weird? |
@ahheo Would you like to update the PR based on the suggestions made? |
Hi, I lead the user support group for Derecho, and I just wanted to say we will really value this contribution. WRF-Chem has been quite painful, so far. Can I also ask a naïve question, in hopes of getting pointed in a useful direction? Thanks again, this is great work. |
@weiwangncar Sorry that for the late reply because I just come back from holidays to office today. The PR was updated as suggested. Please see and review the commit 911facc. |
This reverts commit 82447aa. modified: compile modified: tools/gen_wrf_io.c
…an if condition grouping (tools/gen_wrf_io.c)
@islas Can you take hopefully a final check on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For WRF-Chem since Jordan Schnell has approved this before.
@ahheo If you could let us where you work, we can add it to the PR. Thanks for contributing to our model development effort. |
I work in National Space Science Center, CAS. Feel free to add it or not as I made the changes on my own :). |
@ahheo OK, thanks for letting us know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm glad you're clarifying the precedence of the bit-mask and logical-not operators, I have some questions about the choice of how to do so, and would appreciate hearing the reasons for responding to the warning by insisting you meant the behavior the compiler thought unlikely?
@@ -377,7 +377,7 @@ fprintf(fp," ngrid%%parent_grid_ratio, ngrid%%parent_grid_ratio | |||
strcpy( tmpstr , pp->interpu_aux_fields ) ; | |||
} else if ( down_path & FORCE_DOWN ) { | |||
/* by default, add the boundary and boundary tendency fields to the arg list */ | |||
if ( ! p->node_kind & FOURD ) { | |||
if ( (! p->node_kind) & FOURD ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( (! p->node_kind) & FOURD ) { | |
if ( ! (p->node_kind & FOURD) ) { |
or
if ( (! p->node_kind) & FOURD ) { | |
if ( (~ p->node_kind) & FOURD ) { |
registry.h
has #define FOURD 8
so this conditional will never be met (!p->node_kind)
will be 0 or 1)
@@ -85,7 +85,7 @@ gen_wrf_io2 ( FILE * fp , char * fname, char * structname , char * fourdname, no | |||
{ | |||
|
|||
|
|||
if ( p->ndims > 3 && ! p->node_kind & FOURD ) continue ; /* short circuit anything with more than 3 dims, (not counting 4d arrays) */ | |||
if ( p->ndims > 3 && ( (! p->node_kind) & FOURD ) ) continue ; /* short circuit anything with more than 3 dims, (not counting 4d arrays) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( p->ndims > 3 && ( (! p->node_kind) & FOURD ) ) continue ; /* short circuit anything with more than 3 dims, (not counting 4d arrays) */ | |
if ( p->ndims > 3 && ! (p->node_kind & FOURD) ) continue ; /* short circuit anything with more than 3 dims, (not counting 4d arrays) */ |
or
if ( p->ndims > 3 && ( (! p->node_kind) & FOURD ) ) continue ; /* short circuit anything with more than 3 dims, (not counting 4d arrays) */ | |
if ( p->ndims > 3 && (~ p->node_kind) & FOURD ) continue ; /* short circuit anything with more than 3 dims, (not counting 4d arrays) */ |
registry.h
has #define FOURD 8
so this conditional will never be met (!p->node_kind)
will be 0 or 1)
@@ -1058,7 +1058,7 @@ check_dimspecs() | |||
p->assoc_nl_var_s,p->name ) ; | |||
return(1) ; | |||
} | |||
if ( ! q->node_kind & RCONFIG ) | |||
if ( (! q->node_kind) & RCONFIG ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( (! q->node_kind) & RCONFIG ) | |
if ( ! (q->node_kind & RCONFIG) ) |
or
if ( (! q->node_kind) & RCONFIG ) | |
if ( (~ q->node_kind) & RCONFIG ) |
I suspect RCONFIG != 1
, so this conditional is unlikely to be met as-is.
@@ -1083,7 +1083,7 @@ check_dimspecs() | |||
p->assoc_nl_var_e,p->name ) ; | |||
return(1) ; | |||
} | |||
if ( ! q->node_kind & RCONFIG ) | |||
if ( (! q->node_kind) & RCONFIG ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( (! q->node_kind) & RCONFIG ) | |
if ( ! (q->node_kind & RCONFIG) ) |
or
if ( (! q->node_kind) & RCONFIG ) | |
if ( (~ q->node_kind) & RCONFIG ) |
I suspect RCONFIG != 1
, so this conditional will never be met as-is.
@DWesl Thank you for raising the questions. I would point out that my corresponding changes focused only on the grouping of if-conditions but not the precedence of the bit-mask and logical-not operators. Assuming that the original code yields warnings but gives the right results, the rule applied for grouping is to have the same result after grouping. Indeed, I have made the following test (currently with only icx compiler):
But I cannot confirm that the assumption is reasonable, and I wouldn't say if the warnings are related to the ifx compiler. Finally, it would be very appreciated if you can validate the assumption and/or made changes with your reasons provided. |
Lines 27 to 28 in 794843f
I think if you use either value for |
@DWesl I agree with you. Well, I have not idea what I need to do at this point as the pull request has been merged already. |
…ags (#2030) Fix issue with generalized compiler version output using the wrong flags TYPE: bug fix KEYWORDS: compile, version SOURCE: internal DESCRIPTION OF CHANGES: Problem: A generalized compile version check was proposed in #1987. This logic was implemented in #1942 but the originating logic contains a bug where the `-V` and `--version` flag commands' output is flipped. Solution: Use `-V` in the appropriate spot when `$status` is zero for the `-V` check, and respectively for the `--version` check. LIST OF MODIFIED FILES: M compile TESTS CONDUCTED: 1. With previous bad logic the compile log output shows a compiler error as the wrong flag is used to output version info. With the fix, the correct output now shows in the compile log.
…ags (wrf-model#2030) Fix issue with generalized compiler version output using the wrong flags TYPE: bug fix KEYWORDS: compile, version SOURCE: internal DESCRIPTION OF CHANGES: Problem: A generalized compile version check was proposed in wrf-model#1987. This logic was implemented in wrf-model#1942 but the originating logic contains a bug where the `-V` and `--version` flag commands' output is flipped. Solution: Use `-V` in the appropriate spot when `$status` is zero for the `-V` check, and respectively for the `--version` check. LIST OF MODIFIED FILES: M compile TESTS CONDUCTED: 1. With previous bad logic the compile log output shows a compiler error as the wrong flag is used to output version info. With the fix, the correct output now shows in the compile log.
…ags (wrf-model#2030) Fix issue with generalized compiler version output using the wrong flags TYPE: bug fix KEYWORDS: compile, version SOURCE: internal DESCRIPTION OF CHANGES: Problem: A generalized compile version check was proposed in wrf-model#1987. This logic was implemented in wrf-model#1942 but the originating logic contains a bug where the `-V` and `--version` flag commands' output is flipped. Solution: Use `-V` in the appropriate spot when `$status` is zero for the `-V` check, and respectively for the `--version` check. LIST OF MODIFIED FILES: M compile TESTS CONDUCTED: 1. With previous bad logic the compile log output shows a compiler error as the wrong flag is used to output version info. With the fix, the correct output now shows in the compile log.
Resolving building issues with Intel compilers (ifx/icx) for WRF/WRF-chem
TYPE: bug fix
KEYWORDS: Missing/Wrong Prototypes
SOURCE: Changgui Lin
DESCRIPTION OF CHANGES:
Problem:
Problem: Most C code written long ago. No prototypes were used. See #1823. And, this pr is kind of an extension to #1823 addressing the same issue for building WRF-chem.
Solution:
Add missing prototypes; rearrange function order.
LIST OF MODIFIED FILES: list of changed files (use
git diff --name-status master
to get formatted list)modified: arch/configure.defaults
modified: chem/KPP/kpp/kpp-2.1/src/code.c
modified: chem/KPP/kpp/kpp-2.1/src/code.h
modified: chem/KPP/kpp/kpp-2.1/src/code_c.c
modified: chem/KPP/kpp/kpp-2.1/src/code_f77.c
modified: chem/KPP/kpp/kpp-2.1/src/code_f90.c
modified: chem/KPP/kpp/kpp-2.1/src/code_matlab.c
modified: chem/KPP/kpp/kpp-2.1/src/gdata.h
modified: chem/KPP/kpp/kpp-2.1/src/gen.c
modified: chem/KPP/kpp/kpp-2.1/src/gen_org.c
modified: chem/KPP/kpp/kpp-2.1/src/kpp.c
modified: chem/KPP/kpp/kpp-2.1/src/scan.h
modified: chem/KPP/kpp/kpp-2.1/src/scan.l
modified: chem/KPP/kpp/kpp-2.1/src/scan.y
modified: chem/KPP/util/wkc/Makefile
modified: chem/KPP/util/wkc/change_chem_Makefile.c
modified: chem/KPP/util/wkc/compare_kpp_to_species.c
modified: chem/KPP/util/wkc/gen_kpp.c
modified: chem/KPP/util/wkc/gen_kpp_args_to_Update_Rconst.c
modified: chem/KPP/util/wkc/gen_kpp_interf_utils.c
modified: chem/KPP/util/wkc/gen_kpp_interface.c
modified: chem/KPP/util/wkc/gen_kpp_mech_dr.c
modified: chem/KPP/util/wkc/gen_kpp_utils.c
modified: chem/KPP/util/wkc/get_kpp_chem_specs.c
modified: chem/KPP/util/wkc/get_wrf_jvals.c
modified: chem/KPP/util/wkc/get_wrf_radicals.c
modified: chem/KPP/util/wkc/kpp_data.c
modified: chem/KPP/util/wkc/protos_kpp.h
modified: chem/KPP/util/wkc/registry_kpp.c
modified: chem/KPP/util/write_decomp/integr_edit.c
modified: compile
modified: external/RSL_LITE/buf_for_proc.c
modified: external/RSL_LITE/c_code.c
modified: external/RSL_LITE/cycle.c
modified: external/RSL_LITE/gen_comms.c
modified: external/RSL_LITE/period.c
modified: external/RSL_LITE/rsl_bcast.c
modified: external/RSL_LITE/rsl_lite.h
modified: external/RSL_LITE/rsl_malloc.c
modified: external/RSL_LITE/swap.c
modified: external/RSL_LITE/task_for_point.c
modified: external/io_grib1/MEL_grib1/FTP_getfile.c
modified: external/io_grib1/MEL_grib1/gribfuncs.h
modified: external/io_grib1/MEL_grib1/gribgetbds.c
modified: external/io_grib1/MEL_grib1/gribhdr2file.c
modified: external/io_grib1/MEL_grib1/init_enc_struct.c
modified: external/io_grib1/MEL_grib1/init_gribhdr.c
modified: external/io_grib1/MEL_grib1/init_struct.c
modified: external/io_grib1/MEL_grib1/ld_enc_input.c
modified: external/io_grib1/MEL_grib1/ld_enc_lookup.c
modified: external/io_grib1/MEL_grib1/ld_grib_origctrs.c
modified: external/io_grib1/MEL_grib1/map_lvl.c
modified: external/io_grib1/MEL_grib1/map_parm.c
modified: external/io_grib1/MEL_grib1/prt_badmsg.c
modified: external/io_grib1/MEL_grib1/prt_inp_struct.c
modified: external/io_grib1/MEL_grib1/set_bytes.c
modified: external/io_grib1/MEL_grib1/upd_child_errmsg.c
modified: external/io_grib1/grib1_routines.c
modified: external/io_grib1/grib1_util/read_grib.c
modified: external/io_grib1/grib1_util/read_grib.h
modified: external/io_grib1/grib1_util/write_grib.c
modified: external/io_grib1/gribmap.c
modified: external/io_grib1/gribmap.h
modified: external/io_grib1/trim.c
new file: external/io_grib1/trim.h
modified: external/io_grib_share/get_region_center.c
modified: external/io_grib_share/open_file.c
modified: frame/pack_utils.c
modified: tools/Makefile
modified: tools/gen_config.c
modified: tools/gen_interp.c
modified: tools/gen_irr_diag.c
modified: tools/gen_streams.c
modified: tools/gen_wrf_io.c
modified: tools/misc.c
modified: tools/protos.h
modified: tools/reg_parse.c
modified: tools/sym.c
modified: tools/sym.h
modified: tools/symtab_gen.c