Resolve "Real(8) should go back to Real(dp)" - [merged] #2409

Closed jonmaddock closed 1 year ago

In GitLab by @timothy-nunn on Oct 1, 2021, 14:24

Merges 1406-real-8-should-go-back-to-real-dp-2 -> develop

Closes #1406

In GitLab by @timothy-nunn on Oct 1, 2021, 16:14

added 2 commits

f126e7f2 - Start adding intrinsic guard
a558223e - Completed intrinsic guarding

Compare with previous version

In GitLab by @timothy-nunn on Oct 5, 2021, 13:25

added 1 commit

0b51e4b3 - 1 commit from branch 1397-remove-f90wrap-from-build-process-tim2

Compare with previous version

In GitLab by @timothy-nunn on Oct 5, 2021, 13:31

closed

In GitLab by @timothy-nunn on Oct 5, 2021, 13:31

reopened

In GitLab by @timothy-nunn on Oct 5, 2021, 13:31

changed target branch from 1397-remove-f90wrap-from-build-process-tim2 to develop

In GitLab by @timothy-nunn on Oct 5, 2021, 13:48

added 2 commits

a5c42eb6 - Added some missing non-module intrinsic guards
4f9fe3c4 - Preprocess all wrapped files (in preperation to add back real(dp))

Compare with previous version

In GitLab by @timothy-nunn on Oct 5, 2021, 13:56

The general pattern I am going with here is as follows:

Original

! source/fortran/foo.f90
module foo
    use, intrinsic :: iso_fortran_env, only: dp=>real64
    implicit none

    subroutine bar(x, y, z)
        real(dp), intent(in) :: x, y
        real(dp), intent(out) :: z

        ! very important subroutine logic
    end subroutine bar
end module foo

However when f2py wraps foo it does not include the module-level declaration of dp. Therefore, when gfortran tries to compile the wrapper, it cannot find a definition of dp and errors out.

New

! source/fortran/foo.f90
module foo
#ifndef dp
    use, intrinsic :: iso_fortran_env, only: dp=>real64
#endif
    implicit none

    subroutine bar(x, y, z)
        real(dp), intent(in) :: x, y
        real(dp), intent(out) :: z

        ! very important subroutine logic
    end subroutine bar
end module foo

Therefore, when we compile the fortran, we provide no additional directives. However, we then also preprocess the wrapped fortran files with a -Ddp=8 and get f2py to wrap the preprocessed source files. This way f2py is wrapping:

! build/foo.f90
module foo
    implicit none

    subroutine bar(x, y, z)
        real(8), intent(in) :: x, y
        real(8), intent(out) :: z

        ! very important subroutine logic
    end subroutine bar
end module foo

And there is no dp pointer for it to not be able to find, therefore no error.

In GitLab by @timothy-nunn on Oct 5, 2021, 14:16

added 1 commit

c4c70af3 - Reinstated real(dp) back into all fortran source files

Compare with previous version

In GitLab by @timothy-nunn on Oct 5, 2021, 14:27

Hi Jon,

This MR is getting pretty big what with ~3000 real(8) being replaced with real(dp) so I think I will leave it here.

I would like your opinion on where I have placed the preprocess.py script, currently in the cmake folder, can you think of a better place for it?

In GitLab by @timothy-nunn on Oct 5, 2021, 14:45

added 2 commits

fa68bce9 - Changed intrensic guard in fortran to check for already declared dp rather...
bce04aa1 - use_intrinsic directive no longer required in fortran compilation

Compare with previous version

In GitLab by @timothy-nunn on Oct 5, 2021, 14:55

A higher-level overview of the process is:

On each cmake we create a build context file preprocess.txt which holds information about files to wrap, source and destination directories, and directives (the preprocess context).

The new python preprocess.py script is able to parse this context file and creates a subprocess which preprocesses a given source file into an equivalently named source file in the build directory.

f2py will then wrap these preprocessed source files instead of the "raw" source files.

In GitLab by @timothy-nunn on Oct 5, 2021, 15:06

added 12 commits

bce04aa1...b5ab7988 - 11 commits from branch develop
6a75909b - Merge branch 'develop' into 1406-real-8-should-go-back-to-real-dp-2

Compare with previous version

In GitLab by @timothy-nunn on Oct 5, 2021, 15:15

unmarked as a Work In Progress

In GitLab by @timothy-nunn on Oct 7, 2021, 16:48

On-hold until develop-stable is removed

In GitLab by @timothy-nunn on Nov 1, 2021, 10:39

added 208 commits

6a75909b...94a0ea59 - 207 commits from branch develop
7810019c - Merge branch 'develop' into 1406-real-8-should-go-back-to-real-dp-2

Compare with previous version

In GitLab by @timothy-nunn on Nov 1, 2021, 10:42

added 1 commit

1e17679b - Resolve missed merge conflict in sctfcoil.f90

Compare with previous version

In GitLab by @timothy-nunn on Nov 1, 2021, 15:20

added 54 commits

1e17679b...4488ce7d - 53 commits from branch develop
e1adf40c - Merge branch 'develop' into 1406-real-8-should-go-back-to-real-dp-2

Compare with previous version

In GitLab by @timothy-nunn on Nov 1, 2021, 15:32

added 1 commit

4afd5716 - Remove output_module.f90

Compare with previous version

I think these commented out lines can be deleted.

My cmake knowledge is not great! We want the ${PREPROCESS_ALL_NAME} target to run before the custom command outputting ${F2PY_TARGET} ${F2PY_OUTPUT}; i.e. we want the preprocessing to be done before f2py gets run. I'm not totally sure that this is guaranteed to be the case; are ${F2PY_TARGET} ${F2PY_OUTPUT} ${PREPROCESS_ALL_NAME} free to run in any order currently? Perhaps I'm paranoid about cmake race conditions, but could you convince me this is alright?

I think the "-Duse_intrinsic" and "-cpp" options can go now, right? All preprocessing is being done in the Python script using context.txt?

In GitLab by @timothy-nunn on Nov 8, 2021, 11:55

Commented on cmake/f2py.cmake line 22

I agree. I will look at adding a depends to the actual command that runs f2py, just to be safe. I believe that CMake figures it out but don't quote me on that.

In GitLab by @timothy-nunn on Nov 8, 2021, 11:56

Commented on cmake/f2py.cmake line 27

Maybe not -cpp but certainly -Duse_intrinsic can go

@tim First of all, well done for figuring out that we could continue to compile with gfortran using real(dp) and dp=>real64 but use a preprocessor definition of -Ddp=8 to preprocess the source to real(8) so that f2py is happy to wrap the source files. Then ensuring that the f2py-produced module and the gfortran-produced shared object still work together! That's excellent.

It appears to me that the only reason for you creating the preprocess.py script is purely so that each Fortran source file can be preprocessed to create a preprocessed source file of the same name in the build directory (please correct me if I'm wrong). Could this not be achieved more simply using the cmake foreach loop?

I'm happy with all these other changes, but I would prefer if this could be accomplished in cmake rather than exporting cmake vars into the preprocess.txt file, parsing it in a Python script, then running gfortran subprocesses from there. If it can be achieved by looping in cmake, I feel like this would reduce the complexity of the solution and be easier to understand. However if it is necessarily complex, then I'm happy with this.

Haven't the source files been preprocessed already, so the -cpp isn't required here?

In GitLab by @timothy-nunn on Nov 8, 2021, 14:43

Commented on cmake/f2py.cmake line 27

That is right, both flags are not needed

In GitLab by @timothy-nunn on Nov 8, 2021, 14:46

@jonmaddock

I did look into doing this with a forloop in CMake and ran into an issue -- I cannot remember what it was. Given this was at the start of my preprocessing endeavour it may well be that the issues I was facing were not related to CMake, therefore I will experiment with this tomorrow and get back to you.

Tim

In GitLab by @timothy-nunn on Nov 9, 2021, 11:48

Commented on cmake/f2py.cmake line 15

changed this line in version 11 of the diff

In GitLab by @timothy-nunn on Nov 9, 2021, 11:48

Commented on cmake/f2py.cmake line 22

changed this line in version 11 of the diff

In GitLab by @timothy-nunn on Nov 9, 2021, 11:48

Commented on cmake/f2py.cmake line 27

changed this line in version 11 of the diff

In GitLab by @timothy-nunn on Nov 9, 2021, 11:48

added 17 commits

4afd5716...5d644ab9 - 14 commits from branch develop
b54159f8 - Replace external python script with cmake macro/targets for file preprocessing
9a36ca20 - Merge branch 'develop' into 1406-real-8-should-go-back-to-real-dp-2
47f2fc8b - Add real(dp) back to plasmod.f90

Compare with previous version

In GitLab by @timothy-nunn on Nov 9, 2021, 11:50

Commented on cmake/f2py.cmake line 22

I have added a DEPENDS ${PREPROCESS_TARGET_NAMES} to the f2py custom commands to ensure the preprocessing occurs first

In GitLab by @timothy-nunn on Nov 9, 2021, 11:52

@jonmaddock

I have replaced the preprocess.py script with a cmake macro and ensured this macro is run before f2py. See my comments on the threads for more detail.

I have also resolved the merge conflicts.

Tim

Done in cmake now.

resolved all threads

Great!

merged

mentioned in commit 9da83aa1f5917b01546b6d375de2781b17a1a827

ukaea / PROCESS

Resolve "Real(8) should go back to Real(dp)" - [merged] #2409

Original

New