Skip to content
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.

Sign up for GitHub

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

Case insensitive F90 compile rule #2057

Merged
merged 5 commits into from
Oct 11, 2024
Merged

Case insensitive F90 compile rule #2057

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b7f31dc
2024 Updates and Bug Fixes for the P-X LSM (#2023)
coastwx Mar 27, 2024
754f0e5
Merge branch 'master' of github.com:wrf-model/WRF into develop
islas May 9, 2024
2948495
arch/configure.defaults : Removed -qopenmp from OMPCC for Mac with c…
MicroTed Jun 3, 2024
afb7c89
Reverted configure.defaults to be able to make a separate PR with that.
MicroTed Oct 8, 2024
46475a4
Changed .F90 rule to use WRF default for .F with '.tmp.f90' as tempo…
MicroTed Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion arch/configure.defaults
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ DESCRIPTION = INTEL ($SFC/$SCC)
DMPARALLEL = # 1
OMPCPP = # -D_OPENMP
OMP = # -qopenmp -fpp -auto
OMPCC = # -qopenmp
OMPCC = #
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be mistaken, but I think clang does support OpenMP on Apple systems, though the flag is -fopenmp rather than -qopenmp (which may have simply been copied from the Intel Fortran compiler option to enable OpenMP).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default built-in Xcode clang does not support the -fopenmp flag (throws an error). Users can install clang (or gcc) with homebrew, and that does support it, but I would think that users would not expect to have to do that. Is there much C code in the model that uses openmp, anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are fair points, and I don't think there's much C code in WRF that uses OpenMP. Especially since this change only affects Intel-based macOS systems, I'm fine with the change as it is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let others (@weiwangncar, @islas) weigh in on whether this change should appear in a separate PR or commit since it doesn't strictly relate to case-insensitive *.F90 compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apple clang doesn't support openmp on either intel or arm64 macs. (It is possible to run ifort on an M-series mac in emulation, and it actually runs OK.) I didn't think to update the gfortran+clang section since I've never used it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the arch/configure.defaults changes to be separate, as that would also make recording / release notes easier to write each up as separate fixes

SFC = ifort
SCC = clang
CCOMP = clang
Expand Down
13 changes: 1 addition & 12 deletions arch/postamble
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,7 @@ wrfio_esmf :

.F90.o:
$(RM) $@
sed -e "s/^\!.*'.*//" -e "s/^ *\!.*'.*//" $*.F90 > $*.G
$(CPP) -I$(WRF_SRC_ROOT_DIR)/inc $(CPPFLAGS) $(OMPCPP) $*.G > $*.bb
$(SED_FTN) $*.bb | $(CPP) $(TRADFLAG) > $*.f90
$(RM) $*.G $*.bb
$(FC) -o $@ -c $(FCFLAGS) $(OMP) $(MODULE_DIRS) $(PROMOTION) $(FCSUFFIX) $*.f90
$(FC) -o $@ -c -I$(WRF_SRC_ROOT_DIR)/inc $(OMPCPP) $(FCFLAGS) $(ARCHFLAGS) $(OMP) $(MODULE_DIRS) $(PROMOTION) $(FCSUFFIX) $*.F90
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under the assumption that there is a good reason for explicitly processing source files with cpp and sed, would it be safer to preserve that preprocessing, but to instead name the intermediate .f90 files something like .tmp.f90?
I think something like this might be possible (pending testing with a broader range of compilers):

.F90.o:
        $(RM) $@
        sed -e "s/^\!.*'.*//" -e "s/^ *\!.*'.*//" $*.F90 > $*.G
        $(CPP) -I$(WRF_SRC_ROOT_DIR)/inc $(CPPFLAGS) $(OMPCPP) $*.G  > $*.bb
        $(SED_FTN) $*.bb | $(CPP) $(TRADFLAG) > $*.tmp.f90
        $(RM) $*.G $*.bb
        $(FC) -o $@ -c $(FCFLAGS) $(OMP) $(MODULE_DIRS) $(PROMOTION) $(FCSUFFIX) $*.tmp.f90

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the history of WRF compiling to use the cpp/sed etc., but it seems like a lot of extra effort when the built-in fpp should just work. (Maybe the old cray compilers were cranky?) A mangled name should work, too, though, if that would be preferable. I just thought it would be much simpler to just compile it straight. I have a bunch of preprocessing in my codes and haven't run into any problems with built-in fpp, but I also haven't used all possible fortran compilers.

The 'clean' function is the only thing that might need to be updated?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to stick with what has been known to work for WRF. In MPAS, we do let the Fortran compiler handle pre-processing of source files by default, but I seem to recall that WRF code contains some non-standard-isms that necessitated special pre-processing for some compilers.

I don't think anything needs to be modified for the 'clean' script, as that appears to have logic already to remove .f90 files where they are know to have been generated through explicit pre-processing: https://github.com/wrf-model/WRF/blob/v4.6.0/clean#L6-L16 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. The current codes in physics_mmm have hardly any preprocessing, but there's no guarantee of that always being the case.


.F.f90:
$(RM) $@
Expand All @@ -218,13 +214,6 @@ wrfio_esmf :
$(CPP) -I$(WRF_SRC_ROOT_DIR)/inc $(CPPFLAGS) $*.H > $@
$(RM) $*.G $*.H

.F90.f90:
$(RM) $@
sed -e "s/^\!.*'.*//" -e "s/^ *\!.*'.*//" $*.F90 > $*.G
$(SED_FTN) $*.G > $*.H
$(CPP) -I$(WRF_SRC_ROOT_DIR)/inc $(CPPFLAGS) $*.H > $@
$(RM) $*.G $*.H

.f90.o:
$(RM) $@
$(FC) -o $@ -c $(FCFLAGS) $(PROMOTION) $(FCSUFFIX) $*.f90
Expand Down