extending dependency trafo to access specs

[MichaelSt98]

rename/transform private/public access specifiers for modules

@reuterbal and @mlange05: asking for feedback on this

problem for OMNI:

        # This is a call to a subroutine declared via header-included interface
        item_name = f'#{proc_name}'.lower()
        if self._is_ignored(item_name, config, ignore):
            return None
        if config and config.is_disabled(item_name):
            return None
        if item_name not in self.item_cache:
            if not config or config.default.get('strict', True):
>               raise RuntimeError(f'Procedure {item_name} not found in self.item_cache.')
E               RuntimeError: Procedure #kernel_3 not found in self.item_cache.

[github-actions]

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/436/index.html

[reuterbal]

Looks conceptually great. Just some pointers for the implementation

[reuterbal]

The docstring doesn't tell much... But I would make this utility even more basic, something like

Suggested change

	def rename_access_specs(self, module, targets=None, active_nodes=None):
	"""
	Update/rename access specifiers.
	Parameters
	----------
	module : :any:`Module`
	The IR object to transform
	targets : list of str
	Optional list of subroutine names for which to modify access specs
	active_nodes : list of str
	Optional list of active nodes
	"""
	if module.public_access_spec:
	if active_nodes is not None:
	new_access_spec = tuple(elem for elem in module.public_access_spec if elem in active_nodes)
	else:
	new_access_spec = module.public_access_spec
	module.public_access_spec = tuple(f'{elem}{self.suffix}' if not targets or elem in targets else
	elem for elem in new_access_spec)
	if module.private_access_spec:
	if active_nodes is not None:
	new_access_spec = tuple(elem for elem in module.private_access_spec if elem in active_nodes)
	else:
	new_access_spec = module.public_access_spec
	module.private_access_spec = tuple(f'{elem}{self.suffix}' if not targets or elem in targets
	else elem for elem in new_access_spec)
	def rename_access_spec_names(self, access_spec, targets=None, active_nodes=None):
	"""
	Rename target names in an access spec
	For all names in the access spec that are contained in :data:`targets`, rename them as
	``{name}{self.suffix}``. If :data:`active_nodes` are given, then all names
	that are not in the list of active nodes, are being removed from the list.
	Parameters
	----------
	access_spec : list of str
	List of names from an access spec
	targets : list of str
	Optional list of subroutine names for which to modify access specs
	active_nodes : list of str
	Optional list of active nodes
	"""
	if active_nodes:
	access_spec = tuple(elem for elem in access_spec if elem in active_nodes)
	return tuple(
	f'{elem}{self.suffix}' if not targets or elem in targets
	else elem
	for elem in access_spec
	)

And then use that utility simply as:

if module.public_access_spec:
    module.public_access_spec = self.rename_access_spec(
        module.public_access_spec, targets=targets, active_nodes=active_nodes
    )

if module.private_access_spec:
    module.private_access_spec = self.rename_access_spec(
        module.private_access_spec, targets=targets, active_nodes=active_nodes
    )

[reuterbal]

This should likely be module.private_access_spec?

[reuterbal]

Do we not want the enrichment, here?

Suggested change

	driver = Sourcefile.from_source(driver_fcode, frontend=frontend, xmods=[tmp_path])
	driver = Sourcefile.from_source(driver_fcode, frontend=frontend, xmods=[tmp_path], definitions=kernel.definitions)

[codecov]

Codecov Report

Attention: Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.29%. Comparing base (a26cb44) to head (07e81d7).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
loki/transformations/build_system/dependency.py	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
+ Coverage   93.28%   93.29%   +0.01%     
==========================================
  Files         221      221              
  Lines       41262    41360      +98     
==========================================
+ Hits        38491    38587      +96     
- Misses       2771     2773       +2     

Flag	Coverage Δ
lint_rules	96.39% <ø> (ø)
loki	93.25% <98.11%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[reuterbal]

Thanks, looks great! While looking through this I noticed a potential problem when global variables (that are not represented as items in the scheduler graph) are part of the accessibility statement. I'm wondering if we might accidentally remove them and thus cause problems, see comments in code.

A simple "solution" might be, when building a list of active nodes to make sure we also keep any module variables that are marked as public in there

[reuterbal]

Suppose we have a global variable defined in the access spec, for example changing your test to look like this:

INTEGER :: some_const
PRIVATE
PUBLIC kernel, kernel_2, unused_kernel, some_const

Would this variable be removed from the access spec list here? Because global variables are never represented by dedicated items, they would never show up in the active_nodes list. I suspect this could be a potential problem?

[reuterbal]

Related question here (sorry, made that mental connection only now): if we only use a global variable from a module, we would have the corresponding ModuleItem in the scheduler graph, but nothing in the module would be registered as active nodes.

In that situation, I would expect kwargs['items'] is an empty tuple rather than None, thus allowing to distinguish between the situation where that argument isn't provided, vs. a situation where it is provided but empty.

In l.137 that check is correct in my opinion, which would yield active_nodes = []. We might need to reflect that distinction here, too, by changing the conditional like this (correct me if I'm wrong here!):

Suggested change

	if active_nodes:
	if active_nodes is not None:

[MichaelSt98]

Agreed ... @reuterbal: Don't we have a problem/bug even without this PR?

Executing:

from loki import fgen, DependencyTransformation, Sourcefile

if __name__ == "__main__":
    kernel_fcode = """
MODULE kernel_access_spec_mod

  INTEGER, PUBLIC :: some_const
  INTEGER :: another_const

PRIVATE
PUBLIC kernel, kernel_2, unused_kernel, another_const
CONTAINS
    SUBROUTINE kernel(a, b, c)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a, b, c

    call kernel_2(a, b)
    call kernel_3(c)
  END SUBROUTINE kernel
  SUBROUTINE kernel_2(a, b)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a, b

    a = 1
    b = 2
  END SUBROUTINE kernel_2
  SUBROUTINE kernel_3(a)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a

    a = 3
  END SUBROUTINE kernel_3
  SUBROUTINE unused_kernel(a)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a

    a = 3
  END SUBROUTINE unused_kernel
END MODULE kernel_access_spec_mod
    """.strip()

    driver_fcode = """
SUBROUTINE driver(a, b, c)
    USE kernel_access_spec_mod, only: kernel, another_const
    USE kernel_access_spec_mod, only: some_const
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a, b, c

    CALL kernel(a + some_const, b + another_const ,c)
END SUBROUTINE driver
    """.strip()

    transformation = DependencyTransformation(suffix='_test', module_suffix='_mod')

    kernel = Sourcefile.from_source(kernel_fcode)
    driver = Sourcefile.from_source(driver_fcode, definitions=kernel.definitions)

    kernel.apply(transformation, role='kernel')
    driver['driver'].apply(transformation, role='driver', targets=('kernel', 'kernel_access_spec_mod'))

    print(f"{fgen(driver)}")
    print(f"------------------------")
    print(f"{fgen(kernel)}")

yields:

SUBROUTINE driver (a, b, c)
  USE kernel_access_spec_test_mod, ONLY: kernel_test
  USE kernel_access_spec_mod, ONLY: another_const
  USE kernel_access_spec_mod, ONLY: some_const
  IMPLICIT NONE
  INTEGER, INTENT(INOUT) :: a, b, c
  
  CALL kernel_test(a + some_const, b + another_const, c)
END SUBROUTINE driver
------------------------
MODULE kernel_access_spec_test_mod
  
  private
  PUBLIC :: kernel, kernel_2, unused_kernel, another_const
  INTEGER, PUBLIC :: some_const
  INTEGER :: another_const
  

  CONTAINS
  SUBROUTINE kernel_test (a, b, c)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a, b, c
    
    CALL kernel_2(a, b)
    CALL kernel_3(c)
  END SUBROUTINE kernel_test
  SUBROUTINE kernel_2_test (a, b)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a, b
    
    a = 1
    b = 2
  END SUBROUTINE kernel_2_test
  SUBROUTINE kernel_3_test (a)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a
    
    a = 3
  END SUBROUTINE kernel_3_test
  SUBROUTINE unused_kernel_test (a)
    IMPLICIT NONE
    INTEGER, INTENT(INOUT) :: a
    
    a = 3
  END SUBROUTINE unused_kernel_test
END MODULE kernel_access_spec_test_mod

Is

  USE kernel_access_spec_test_mod, ONLY: kernel_test
  USE kernel_access_spec_mod, ONLY: another_const
  USE kernel_access_spec_mod, ONLY: some_const

what we want to have?

[reuterbal]

Excellent question, I think that depends on the transformed code. Afaik this situation occurs in ecWAM, where you want to duplicate the module routines to supply offload code, but you don't want to duplicate the module variables because they are also used outside in host code. Instead, data offload is applied to the original module, without duplicating. Therefore, the outcome you describe looks correct to me. This may, of course, incur problems if the module variables are used by module procedures.

For the scope of this PR, I would say as long as it retains current behaviour that's fine.

For the future, we should support two modes for these situations.
Consider an example like this:

module a_mod
    implicit none
    private
    public :: var, sub
    integer :: var
contains
    subroutine sub(arg)
        integer, intent(inout) :: arg
        arg = arg + var
    end subroutine sub
end module a_mod

First mode: global module variables are replicated, i.e., we keep them in the public access spec.

module a_loki_mod
    implicit none
    private
    public :: var, sub_loki
    integer :: var
contains
    subroutine sub_loki(arg)
        integer, intent(inout) :: arg
        arg = arg + var
    end subroutine sub_loki
end module a_loki_mod

Second mode: global variables are kept in the original module and used from there, i.e., the declaration is replaced by an import:

module a_loki_mod
    use a_mod, only: var
    implicit none
    private
    public :: sub_loki
contains
    subroutine sub_loki(arg)
        integer, intent(inout) :: arg
        arg = arg + var
    end subroutine sub
end module a_loki_mod

This declaration-to-import change should probably be a separate transformation. Something similar would also be useful for the derived type definitions in ecrad.

[MichaelSt98]

Is it necessary to take special care of module.typedefs and module.interfaces regarding access specifiers?

[reuterbal]

Oh, good point. Typedefs presumably in the same way as variables, interfaces should be treated like procedures (so no special handling required).

[MichaelSt98]

Oh, good point. Typedefs presumably in the same way as variables, interfaces should be treated like procedures (so no special handling required).

Ok, I'll add a commit to take care of typedefs as well.

[reuterbal]

Many thanks, looks great!

[mlange05]

Looks great! Many thanks to both for the thorough review and implementation. GTG