adds check for intel version to add -recursive flag

[pardallio]

Description

While compiling with intel 19.x, I got this error:

/lustre/scratch/pardalm/IAL/build/field_api/host_alloc_module.F90(221): error #6437: A subroutine or function is calling itself recursively. [MEM_FREE] IF( ASSOCIATED(BLK%NEXT) ) CALL SELF%MEM_FREE(BLK%NEXT) ----------------------------------------^ /lustre/scratch/pardalm/IAL/build/field_api/host_alloc_module.F90(260): error #6437: A subroutine or function is calling itself recursively. [REQUEST_FREE] CALL SELF%REQUEST_FREE(FIELD_BLKID, BLK%NEXT, BLKID) ----------------^ /lustre/scratch/pardalm/IAL/build/field_api/host_alloc_module.F90(281): error #6437: A subroutine or function is calling itself recursively. [REQUEST_MEM] CALL SELF%REQUEST_MEM(ALLOC_SIZE, BLK%NEXT, DATA, BLKID)

This problem can be solved by adding a -recursive flag when compiling, but it is only needed for intel 19.x. For >20.x it is no longer a problem, so I added check in compile flags.

Not sure what's the procedure to test

Contributor Declaration

By opening this pull request, I affirm the following:

• All authors agree to the Contributor License Agreement.
• The code follows the project's coding standards.
• I have performed self-review and added comments where needed.
• I have added or updated tests to verify that my changes are effective and functional.
• I have run all existing tests and confirmed they pass.

[awnawab]

Hi @pardallio. Thanks for this contribution. The change itself looks fine, apart from the whitespace being off and a merge conflict which needs a rebase to resolve.

Do you need to build with Intel 19 as part of a bundle or do you need a standalone build? If you need a standalone build then we can merge this PR once you've rebased to resolve the merge conflict and aligned the whitespacing.

If this is as part of a bundle build, then I would argue such a generic language feature flag which is compiler version dependent probably belongs in a toolchain? Other projects in the bundle might also use recursive subroutines and would need the same flags, so you could just add it once in the bundle.

set( CMAKE_Fortran_FLAGS "-recursive" )
or
set( ECBUILD_Fortran_FLAGS "-recursive" )

Either of these above should work.

As for testing the change, the field_api CI has a test for the memory pool and this will test if the recursion is working properly.

[pardallio]

It is being compiled as part of the bundle (IAL-bundle).

But it seems field_api is the only project where this is an issue, so it makes sense to add it directly in this project.

Additionally, in which toolchain would we add this flag?

If it's in the external bundle would it not leave field_api unable to be compiled standalone with intel 19?
And if it's in field_api's toolchain it would not affect the bundle's compilation options?

[awnawab]

Ok that's fair enough. If field_api is the only one that needs it, then we should fix it here. I'm a bit hesitant about applying this flag across the repo, since it might have performance implications. If you change the subroutine declarations on line 218 and line 233 of src/core/host_alloc_module.fypp to:

RECURSIVE SUBROUTINE MEM_POOL_REQUEST_FREE
....
RECURSIVE SUBROUTINE MEM_POOL_REQUEST_MEM

Does that also solve your problem? Sadly I can't test the above locally since all versions of the Intel compiler that are available without a license assume recursive subroutines by default.

[pardallio]

changing the subroutine declarations is likely to solve the problem, but it is also more likely to cause unforseen problems, since that change would be there regardless of the compiler being used.

Adding the flag when intel19 is used only changes something if intel19 is being used, and it will not impact anything if other compilers/version are used.

I'm guessing it will not lead to any degradation from the current status of ectrans compiled with intel19, since it doesn't even compile currently .

[awnawab]

The RECURSIVE keyword as per the standard is redundant for F2018 compliant compilers. The CI here is very comprehensive, and tests gnu, cray, ifx, ifort, nvhpc, flang, amdflang (also on gpu where relevant). I would prefer the RECURSIVE keyword approach, and if we can get it through the CI then we can have a high degree of confidence that it isn't having unintended consequences.

HOST_ALLOC_MODULE is not a performance sensitive part of the code, but applying it widely across the repo could have a negative impact on performance sensitive parts such as GET_VIEW. If the RECURSIVE keyword throws up problems during the CI, then the backup plan would be to set the -recursive flag for just that source file via set_source_files_properties.