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.

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

Refactor implicit none to type,external #3002

Closed
wants to merge 8 commits into from

Conversation

mathomp4
Copy link
Member

@mathomp4 mathomp4 commented Aug 29, 2024

Types of change(s)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)
  • Refactor (no functional changes, no api changes)

Checklist

  • Tested this change with a run of GEOSgcm
  • Ran the Unit Tests (make tests)

Description

Got a bit bored and wondered "What if I did a mass search-and-replace" of implicit none to implicit none (type, external).

Now let's see what the CI says! → What CI says is "Listen to @jeffhammond and move to use mpi_f08" as many use mpi procedures do not have explicit interfaces!

Oh dear lord it's worse than I thought. Intel MPI required all the external :: MPI_Send junk. But with Open MPI:

[  3%] Building Fortran object shared/CMakeFiles/MAPL.shared.dir/Shmem/Shmem.F90.o
/__w/MAPL/MAPL/shared/Shmem/Shmem.F90:547:23:

  547 |   external :: MPI_Bcast, MPI_AllGather, MPI_AllReduce
      |                       1
Error: Cannot change attributes of USE-associated symbol at (1)

So, since MPI stacks are different with use mpi, we don't use (type, external) when MPI is used in a file.

Likewise, in a few places we use old-school netcdf2 interfaces:

base/MAPL_LlcGridFactory.F90
598:      include 'netcdf.inc'
657:      include 'netcdf.inc'

MAPL_cfio/ESMF_CFIOBaseMod.F90
3:   include "netcdf.inc"

We have issues #1865 and #2327 to remove these. @atrayano can do the latter, but perhaps with @bena-nasa we can get the CFIO one done...unless it's just dead code for MAPL3.

Related Issue

@mathomp4 mathomp4 added the 0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) label Aug 29, 2024
@mathomp4 mathomp4 self-assigned this Aug 29, 2024
@tclune
Copy link
Collaborator

tclune commented Sep 3, 2024

You can't use type,external with use mpi

You can't portably use type, external with use mpi

@mathomp4
Copy link
Member Author

mathomp4 commented Sep 3, 2024

You can't use type,external with use mpi

You can't portably use type, external with use mpi

Ah. Yes. Better to say that. 😄

Good news is, it looks like this is close to passing. So that's nice.

@mathomp4 mathomp4 marked this pull request as ready for review October 15, 2024 19:05
@mathomp4 mathomp4 requested a review from a team as a code owner October 15, 2024 19:05
@mathomp4
Copy link
Member Author

Note to self: I'll probably need to do more of this once this gets into MAPL3.

@mathomp4
Copy link
Member Author

Ooh. All passed!

@mathomp4 mathomp4 marked this pull request as draft October 15, 2024 19:12
@mathomp4
Copy link
Member Author

mathomp4 commented Oct 15, 2024

Well, nuts. I'm drafting for now. Why? nvfortran doesn't support it it looks like:

> cat test.F90
program test
   implicit none (type, external)
   integer, parameter :: n = 5
   print *, n
end
> nvfortran test.F90
NVFORTRAN-S-0034-Syntax error at or near ( (test.F90: 2)
  0 inform,   0 warnings,   1 severes, 0 fatal for test

I'll ping @cponder and ask if maybe there is a flag for nvfortran to support this?

Never mind. I asked on the Forum a while back and the answer is "It won't":

https://forums.developer.nvidia.com/t/support-for-implicit-none-external/290470/3

@tclune
Copy link
Collaborator

tclune commented Oct 15, 2024

I knew there was a reason. OTOH, I think we are confident enough in Flang that we could just accept we won't ever use nvfortran?

Copy link
Contributor

@darianboggs darianboggs left a comment

Choose a reason for hiding this comment

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

I noticed that sometimes implicit none is converted to implicit none (type, external) and other times it is converted to implicit none (type) Why is it different?

@tclune
Copy link
Collaborator

tclune commented Oct 22, 2024

I noticed that sometimes implicit none is converted to implicit none (type, external) and other times it is converted to implicit none (type) Why is it different?

Some of the MPI and NetCDF libraries do not play well with implicit none (external). So Matt had to use the older form (but newer syntax) in those cases.

@tclune
Copy link
Collaborator

tclune commented Oct 22, 2024

And both forms fail with nvfortran so ... not sure where we are going with this PR.

@mathomp4
Copy link
Member Author

One thought we've had is to use a "macro" for the implicit none (type, external) case. As @tclune taught me implicit none (type) is equivalent to implicit none so that's "safe" for all

@mathomp4 mathomp4 closed this Dec 4, 2024
@jeffhammond
Copy link

Do we have nvidia bug report for this already or not?

@tclune
Copy link
Collaborator

tclune commented Dec 5, 2024

Sorry @jeffhammond - this one is hardly a pressing matter for us. My initial thought is that we don't want the nvfortran team wasting time on this issue unless others really need this. We don't realistically expect to get our full model to work with nvfortran and instead are putting our investments (and hopes) into nvflang.

If you have some internal reason for which you wish we'd submit a bug report - just respond one more time, and we will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants