Global var hoisting

[MichaelSt98]

Global/Imported variable hoisting.

Convert e.g.,

module moduleB
   real :: var2
   real :: var3
end module moduleB

module moduleC
   real :: var4
   real :: var5
end module moduleC

subroutine driver()
implicit none

call kernel()

end subroutine driver

subroutine kernel()
use moduleB, only: var2,var3
use moduleC, only: var4,var5
implicit none

var4 = var2
var5 = var3

end subroutine kernel

to:

module moduleB
   real :: var2
   real :: var3
end module moduleB

module moduleC
   real :: var4
   real :: var5
end module moduleC

subroutine driver()
use moduleB, only: var2,var3
use moduleC, only: var4,var5
implicit none

call kernel(var2, var3, var4, var5)

end subroutine driver

subroutine kernel(var2, var3, var4, var5)
implicit none
real, intent(in) :: var2
real, intent(in) :: var3
real, intent(inout) :: var4
real, intent(inout) :: var5

var4 = var2
var5 = var3

end subroutine kernel

Tested locally with CLOUDSC Loki C transpilation. However, this requires PR #208.

[github-actions]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (73c6635) 92.31% compared to head (a840d1d) 92.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
+ Coverage   92.31%   92.36%   +0.04%     
==========================================
  Files          95       95              
  Lines       17368    17478     +110     
==========================================
+ Hits        16034    16144     +110     
  Misses       1334     1334              

Flag	Coverage Δ
lint_rules	96.22% <ø> (ø)
loki	92.31% <ø> (ø)
transformations	91.93% <100.00%> (+0.46%)	⬆️

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, this looks very good to me!

I've left a few suggestions for stylistic improvements and flagged a potential issue when an import shadows an import in a parent scope. Arguably, this is an almost academic edge case and bad practice anyway, but the fix is easy.

[reuterbal]

I find the in-line special casing around OMNI hard to read. Especially, because it's mixed with a different way of distinguishing between OMNI and the others already. Could we maybe pull this out and add a corresponding comment that explains this?

A possible pattern would be this:

if frontend == OMNI:
    # OMNI replaces all parameters by their values. For other frontends we get the
    # parameters as additional analysis data entries
    nfld_dim = '1:3'
    nval_dim = '1:3'
    nfld_data = set()
    nval_data = set()
else:
    nfld_dim = 'nfld'
    nval_dim = 'nval'
    nfld_data = {('nfld', 'global_var_analysis_header_mod')}
    nval_data = {('nval', 'global_var_analysis_header_mod')}

expected_trafo_data = {
    'global_var_analysis_header_mod#nval': {
        ...
        'offload': nval_data,
    }
    ...
    'global_var_analysis_kernel_mod#kernel_a': {
        'defines_symbols': set(),
        'uses_symbols': nval_data | nfld_data | {
            (f'iarr({nfld_dim})', 'global_var_analysis_header_mod'),
            (f'rarr({nval_dim}, {nfld_dim})', 'global_var_analysis_header_mod'),
        },
    ...

[reuterbal]

Wouldn't this achieve the same?

Suggested change

	all_defines_symbols = set()
	all_uses_symbols = set()
	for _, value in defines_symbols.items():
	all_defines_symbols |= value
	for _, value in uses_symbols.items():
	all_uses_symbols |= value
	all_defines_symbols = set.union(*defines_symbols.values(), set()}
	all_uses_symbols = set.union(*uses_symbols.values(), set()}

[reuterbal]

Mind you: this poses the risk of overwriting a shadowed symbol - for example:

module a_mod
    use b_mod, only: a, b, c
contains
    subroutine a_routine
        use c_mod, only c
        ! symbol_map will now contain c => b_mod 
        ! because the parent scope is processed after the inner scope
    end subroutine a_routine
end module a_mod

One way to avoid this would be to use

import_map = CaseInsensitiveDict(
    (s.name, imprt) for imprt in routine.all_imports[::-1] for s in imprt.symbols
)

[reuterbal]

Thanks! Unfortunately, there's still a bit left related to the imports, which may just have been an oversight. Otherwise good to go!

[mlange05]

Only had a brief look, but looks very neat and well tested! GTG from me.