Unique symbols per precision #61
Conversation
|
Note, preprocessing is based on |
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can just create an issue so we deal with this in a future PR.
There was a problem hiding this comment.
Thanks @samhatfield for creating #62 for follow-up.
|
First read over completed. Here is my summary of what you're proposing, as a sanity check:
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. |
I only applied this to some routines, but we could push this through everywhere JPRB is not needed.
Correct. These are arguments that are only needed to set up the grid point domain decomposition.
Yes, this makes TPM_CONSTANTS module independent of JPRB.
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.
It is also enlightening to see which routines and modules are not used for the actual transforms.
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
force-pushed
the
feature/unique_precision_symbols
branch
from
fe646ab
to
e87e825
Compare
|
Rebased on recently merged CY49R1 sync branch |
wdeconinck
force-pushed
the
feature/unique_precision_symbols
branch
from
e87e825
to
3f3ca55
Compare
|
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>
|
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:
***@***.***>
|
There was a problem hiding this comment.
Why do we pass setup_trans0.h through configure? Is it meant to "do nothing" in this case?
There was a problem hiding this comment.
Good observation! I used this to copy files from source dir to build-dir, and indeed in this case changes nothing of the content.
There was a problem hiding this comment.
So maybe this deserves a comment in the cmake script?
|
I finally read through the CMake changes and that looks good to me! I think I'm starting to like CMake. Can Also |
Hi @samhatfield these files unfortunately cannot be added to the common library because they |
|
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. |
|
Good to merge for 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
|
Merged in develop now! Thanks everyone, especially @piotrows for the majority of all work included in this PR. 🙏 |
| set(backend ${_PAR_BACKEND}) | ||
|
|
||
| file(MAKE_DIRECTORY ${destination}) | ||
| file(MAKE_DIRECTORY ${destination}/${backend}) |
There was a problem hiding this comment.
I think this line (132) is not needed.
There was a problem hiding this comment.
You are right, something fishy here, that should be
file(MAKE_DIRECTORY ${destination}/trans_${backend})This is now added to develop with commit 91a51b1
| 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}") |
There was a problem hiding this comment.
Same comment here ("Invoking ...)
| $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src/trans/include/ectrans> | ||
| $<INSTALL_INTERFACE:include/ectrans> | ||
| SOURCES ${ectrans_${prec}_src} | ||
| PUBLIC_INCLUDES $<INSTALL_INTERFACE:include/ectrans> |
There was a problem hiding this comment.
Is this unspecialised path still valid?
There was a problem hiding this comment.
Yes it is still valid as the "common" includes should be there.
There was a problem hiding this comment.
Yes but I mean the install include files, don't they need a subdirectory per variant?
There was a problem hiding this comment.
So maybe this deserves a comment in the cmake script?
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
_spand_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.