PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

mixed precision metadata

Open rupertford opened this issue 3 years ago • 9 comments

At the moment, a lack of type-bound procedure in the metadata implies an interface and the interface information needs to be examined by PSyclone. This does not play nicely with metadata reading and writing as the information is not encapsulated within a single type.

module compute_mod

type :: compute_type
  type(arg_type) :: meta_args(1) = (/arg_type(gh_field, gh_real, gh_read, w3)/)
end type compute_type

interface compute_code
   procedure compute1_code, compute2_code
end interface compute_code

contains

  subroutine compute1_code(a)
    real*4 :: a
  end subroutine compute1_code

  subroutine compute2_code(a)
    real*8 :: a
  end subroutine compute2_code

end module

An alternative would be to capture the interface in the type, although this would result in code replication

module compute_mod

type :: compute_type
  type(arg_type) :: meta_args(1) = (/arg_type(gh_field, gh_real, gh_read, w3)/)
  contains
  generic :: compute_code => compute1_code, compute2_code
  procedure, nopass :: compute1_code, compute2_code
end type compute_type

interface compute_code
   procedure compute1_code, compute2_code
end interface compute_code

contains

  subroutine compute1_code(a)
    real*4 :: a
  end subroutine compute1_code

  subroutine compute2_code(a)
    real*8 :: a
  end subroutine compute2_code

end module

Separately to this, the precision could also be captured in the metadata, e.g. ...

type(arg_type) :: meta_args(1) = (/arg_type(gh_field, gh_real(s,d), gh_read, w3)/)

rupertford avatar Nov 09 '22 20:11 rupertford

The problem is, the 'new' metadata-handling classes are written such that they expect to be able to get everything out of the derived-type definition. Since a kernel interface is separate to this, we have no information on it whatsoever:

https://github.com/stfc/PSyclone/blob/afed0a940a9955fb531fb368f5d3da8fbab6fcdd/src/psyclone/domain/lfric/kernel/common_metadata.py#L137-L155 where fparser2_class is just Fortran2003.Derived_Type_Def - i.e. not the whole Specification-part so we have no information on the interface.

arporter avatar Feb 20 '24 13:02 arporter

Another alternative for the metadata might be to have it capture the name of the interface associated with it, e.g.:

module compute_mod

type :: compute_type
  type(arg_type) :: meta_args(1) = (/arg_type(gh_field, gh_real, gh_read, w3)/)
  character(len=50) :: procedure_interface = "compute_code"
end type compute_type

interface compute_code
   procedure compute1_code, compute2_code
end interface compute_code

contains
  ...

That at least avoids the duplication that Rupert mentioned in his suggestion.

arporter avatar Feb 20 '24 14:02 arporter

Another alternative is simply to list all of the procedures that provide kernel implementations, e.g.:

module compute_mod

type :: compute_type
  type(arg_type) :: meta_args(1) = (/arg_type(gh_field, gh_real, gh_read, w3)/)
  procedure, nopass :: compute1_code, compute2_code
end type compute_type

interface compute_code
   procedure compute1_code, compute2_code
end interface compute_code

contains
 ...

The name of the interface that wraps these routines would be given implicitly by the LFRic naming convention and thus we don't need to specify it. This is better than my "character" suggestion as the metadata does now have full information about the interface and the routines it contains.

arporter avatar Feb 20 '24 14:02 arporter

What do @TeranIvy and @christophermaynard think of my last suggestion?

arporter avatar Feb 20 '24 14:02 arporter

Also,@mo-lottieturner has expressed an interest.

arporter avatar Feb 21 '24 13:02 arporter

At the meeting today (01/03/24) we agreed to go with:

module compute_mod

type :: compute_type
  type(arg_type) :: meta_args(1) = (/arg_type(gh_field, gh_real, gh_read, w3)/)
  procedure, nopass :: compute1_code, compute2_code
end type compute_type

interface compute_code
   procedure compute1_code, compute2_code
end interface compute_code

contains
 ...

The first step is to write this up in the documentation and check that that doesn't reveal any potential problems.

arporter avatar Mar 01 '24 14:03 arporter

Documentation updated: image

arporter avatar Sep 06 '24 13:09 arporter

I am not sure I understand the benefit of this (but it introduces some complexity with the implicit generic name).

The type is still not in the metadata, to know the precision we still have to follow the procedure and find and parse the method. Now we do the same but with one extra jump through the interface. Is this extra jump causing issues? We improved the interface support since this issue was created.

sergisiso avatar Sep 10 '24 07:09 sergisiso

The benefit is that it captures everything within the type which is important because the new metadata handling (that Rupert developed) works by reading and writing the type and nothing else. You're right that we have improved the interface support but I've just found that it does not play nicely with kernel transformations - we mess up the naming of the kernel subroutine and generate invalid code. This was/is a step towards us fixing that.

The problem I found is described here: https://github.com/stfc/PSyclone/issues/2663#issuecomment-2333476598

arporter avatar Sep 10 '24 07:09 arporter