Unique symbols per precision #61

wdeconinck · 2024-02-08T22:32:41Z

The libraries trans_sp and trans_dp have duplicated symbols.
This PR uses some kind of preprocessing of original files to generate files with a precision suffix like _sp and _dp, avoiding the same symbol names, so that in the future a library could link to both trans_sp and trans_dp.

The work in this PR is inherited from PR #55 by @piotrows, and tidied up a bit, but does not yet go so far to introduce wrappers that make a unified interface.

Note, commit 5329c07 in this PR changes the API of setup_trans and setup_trans0 to use arguments with KIND=JPRD only. This could be reverted and/or added in a follow-up PR if desired.

When this PR is merged and a new release is to be used in the IFS, an extra commit in ifs-source is required, which I have prepared and would be ready for review. I will update this Issue with ifs-source status.
ifs-source was using some ectrans internal modules or subroutines without including the interface.
So even without API change in setup_trans, the ifs-source commit would be required.

wdeconinck · 2024-02-08T22:38:16Z

Note, preprocessing is based on sed. On macOS 13 this works but on the CI it does not work yet.
Either a workaround will be needed before merging, or we update the macOS runner. Further investigation needed.

src/trans/algor/interpol_decomp_mod.F90

src/trans/internal/abort_trans_mod.F90

src/trans/internal/dealloc_resol_mod.F90

src/trans/internal/dist_grid_32_ctl_mod.F90

src/trans/internal/eq_regions_mod.F90

src/trans/internal/field_split_mod.F90

src/trans/internal/inigptr_mod.F90

src/trans/external/specnorm.F90

src/trans/internal/set_resol_mod.F90

src/trans/internal/setup_dims_mod.F90

src/trans/internal/shuffle_mod.F90

src/trans/internal/sump_trans_mod.F90

samhatfield · 2024-02-09T10:56:20Z

src/trans/internal/sugaw_mod.F90

@@ -108,6 +107,7 @@ SUBROUTINE SUGAW(KDGL,KM,KN,PL,PW,PANM,PFN)

 ! computations in extended precision for alternative root finding
 ! which also works for associated polynomials (m>0)
+INTEGER, PARAMETER :: JPRH = JPRD


This one I need to think about a bit. It looks like we are removing the option to compute the roots in extended precision. If we don't need this anymore, and double precision is fine, better to be transparent and just replace all instances of JPRH with JPRD.

Yes I don't think we need JPRH. At least parkind1 has not recently compiled JPRH differently.
If agreed by others as well we can follow your proposal.

I can just create an issue so we deal with this in a future PR.

Thanks @samhatfield for creating #62 for follow-up.

src/trans/sedrenames.txt

samhatfield · 2024-02-09T11:34:36Z

First read over completed. Here is my summary of what you're proposing, as a sanity check:

Where possible, PARKIND1 replaced with EC_PARKIND
PWEIGHT and PSTRET arguments to SETUP_TRANS and corresponding module variables in TPM_DISTR, TPM_GEOMETRY made double precision always
PRAD argument to SETUP_TRANS0 made double precision always
SPECNORM PNORM argument made mandatory - Why is this necessary?
ectrans_common library created based on subset of JPRB-agnostic modules
ectrans_{sp/dp} "backend libraries" created with JPRB-dependent modules

I'd like some more time to read the new CMake functionality for generating the ectrans_${prec} libraries, but feel free to respond to the above comments in the mean time.

wdeconinck · 2024-02-13T14:36:20Z

Where possible, PARKIND1 replaced with EC_PARKIND

I only applied this to some routines, but we could push this through everywhere JPRB is not needed.

PWEIGHT and PSTRET arguments to SETUP_TRANS and corresponding module variables in TPM_DISTR, TPM_GEOMETRY made double precision always

Correct. These are arguments that are only needed to set up the grid point domain decomposition.

PRAD argument to SETUP_TRANS0 made double precision always

Yes, this makes TPM_CONSTANTS module independent of JPRB.

SPECNORM PNORM argument made mandatory - Why is this necessary?

It is in fact not necessary for this PR, but disambiguates overload on this argument's KIND of this routine in the future. All optional arguments cannot be disambiguated. In practice this argument is always required. Otherwise what's the point. Trying this change in ifs-source proved that this argument is never omitted. This change could be removed from this PR and introduced later, but it's pretty harmless.

ectrans_common library created based on subset of JPRB-agnostic modules

It is also enlightening to see which routines and modules are not used for the actual transforms.

ectrans_{sp/dp} "backend libraries" created with JPRB-dependent modules

I'd like some more time to read the new CMake functionality for generating the ectrans_${prec} libraries, but feel free to respond to the above comments in the mean time.

Noteworthy is that neither backend libraries ectrans_sp and ectrans_dp link with parkind_sp or parkind_dp because the preprocessor step replaces parkind1 with ec_parkind and JPRB with JPRD or JPRM.

wdeconinck · 2024-02-13T15:21:13Z

Rebased on recently merged CY49R1 sync branch

wdeconinck · 2024-02-13T20:35:33Z

I added commit f77550c to develop branch which updates the macOS runner to macos-13. Having rebased this branch on develop now fixes the 'sed' issues previously present.

Co-authored-by: Sam Hatfield <samhatfield@users.noreply.github.com>

src/trans/internal/trgtol_mod.F90

RyadElKhatibMF · 2024-02-15T11:30:17Z

JPIB is better. JPIA has become too ambiguous.

…

----- Météo-France ----- EL KHATIB Ryad DESR/CNRM/GMAP/ALGO ***@***.*** Fixe : +33 561078466

De: "Sam Hatfield" ***@***.***> À: "ecmwf-ifs/ectrans" ***@***.***> Cc: "ryad el khatib" ***@***.***>, "Mention" ***@***.***> Envoyé: Mercredi 14 Février 2024 16:36:16 Objet: Re: [ecmwf-ifs/ectrans] Unique symbols per precision (PR #61) @samhatfield commented on this pull request. In [ #61 (comment) | src/trans/internal/trgtol_mod.F90 ] : > @@ -472,7 +472,7 @@ SUBROUTINE > TRGTOL_COMM(PGLAT,KF_FS,KF_GP,KF_SCALARS_G,KVSET,& INTEGER(KIND=JPIM) :: ISEND, ITAG, JBLK, JFLD, JK, JL, IFLD, II, IFLDS, INS, INR INTEGER(KIND=JPIM) :: JJ,JI,IFLDT, J -INTEGER(KIND=JPIA) :: JFLD64 +INTEGER(KIND=JPIB) :: JFLD64 This variable was introduced in my CY49R1 [ 635f9bb#diff-d50752e10eeb0069e066a6ac0bd9fea3ed72921d7b40aa07cfb5edca47d6efd2R475 | sync commit ] . I think [ https://github.com/RyadElKhatibMF | @RyadElKhatibMF ] picked JPIA for a reason but not sure why. Can we keep it? — Reply to this email directly, [ #61 (review) | view it on GitHub ] , or [ https://github.com/notifications/unsubscribe-auth/AYEAC4OUHKJ6MARY4CEFUZLYTTKXBAVCNFSM6AAAAABDARCI6WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOBQGYZDKMZZHE | unsubscribe ] . You are receiving this because you were mentioned. Message ID: ***@***.***>

src/trans/CMakeLists.txt

piotrows · 2024-02-19T11:14:56Z

src/trans/CMakeLists.txt

Why do we pass setup_trans0.h through configure? Is it meant to "do nothing" in this case?

Good observation! I used this to copy files from source dir to build-dir, and indeed in this case changes nothing of the content.

So maybe this deserves a comment in the cmake script?

samhatfield · 2024-02-23T11:48:56Z

I finally read through the CMake changes and that looks good to me! I think I'm starting to like CMake.

Can internal/sutrle_mod.F90 be added to the common library?

Also external/setup_trans.F90?

wdeconinck · 2024-02-23T16:18:09Z

I finally read through the CMake changes and that looks good to me! I think I'm starting to like CMake.

Can internal/sutrle_mod.F90 be added to the common library?

Also external/setup_trans.F90?

Hi @samhatfield these files unfortunately cannot be added to the common library because they USE modules that have themselves JPRB. e.g. TPM_FIELDS

samhatfield · 2024-02-23T16:45:32Z

Right that makes sense. That also answers the other question I had, which was why we can't automatically build the list of common files. Because that would require code introspection rather than a simple grep.

samhatfield · 2024-02-23T16:49:10Z

Good to merge for me.

marsdeno

This looks good to me.

* feature/unique_precision_symbols: Add setup_trans0 to ectrans_common setup_trans, setup_trans0 API change, using JPRD kind for arguments Add the new precision independent routines to ectrans_common Make some internal modules and routines precision independent Remove sugawc from compilation Generate libraries with unique symbols Code changes related to unused PARKIND1 JPRB

wdeconinck · 2024-02-28T12:20:27Z

Merged in develop now! Thanks everyone, especially @piotrows for the majority of all work included in this PR. 🙏

piotrows · 2024-02-19T17:32:41Z

src/trans/CMakeLists.txt

+  set(backend     ${_PAR_BACKEND})
+
+  file(MAKE_DIRECTORY ${destination})
+  file(MAKE_DIRECTORY ${destination}/${backend})


I think this line (132) is not needed.

You are right, something fishy here, that should be

file(MAKE_DIRECTORY ${destination}/trans_${backend})

This is now added to develop with commit 91a51b1

src/trans/CMakeLists.txt

piotrows · 2024-02-19T18:00:45Z

src/trans/CMakeLists.txt

+      get_filename_component(outfile_ext     ${file_i} EXT)
+      get_filename_component(outfile_dir     ${file_i} DIRECTORY)
+      set(outfile "${destination}/${file_i}")
+      ecbuild_debug("Generate ${outfile}")


Same comment here ("Invoking ...)

src/trans/CMakeLists.txt

piotrows · 2024-02-19T18:19:30Z

src/trans/CMakeLists.txt

-                       $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src/trans/include/ectrans>
-                       $<INSTALL_INTERFACE:include/ectrans>
+      SOURCES          ${ectrans_${prec}_src}
+      PUBLIC_INCLUDES  $<INSTALL_INTERFACE:include/ectrans>


Is this unspecialised path still valid?

Yes it is still valid as the "common" includes should be there.

Yes but I mean the install include files, don't they need a subdirectory per variant?

piotrows · 2024-02-20T15:56:42Z

src/trans/CMakeLists.txt

So maybe this deserves a comment in the cmake script?

wdeconinck requested review from samhatfield, piotrows and marsdeno February 8, 2024 22:32