(Towards #2271, closes #2278) Generalise reference_accesses() and use to tidy KernelModuleInlineTrans.

[arporter]

I found I needed some of this in #2716 (while improving KernelModuleInlineTrans) so I thought I'd best do it as a separate PR.

[arporter]

I need to add explicit unit tests for the new reference_access() methods that I've added to Literal, CodeBlock and ScopingNode. For the moment, my implementation for ScopingNode only adds info about Symbols that appear in precision or initialisation expressions. I rely on the subsequent walk down the tree to add Symbols that are actually used (and also how they are used).

[codecov]

Codecov Report

Attention: Patch coverage is 98.73418% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.88%. Comparing base (8736ae3) to head (b568451).

Files with missing lines	Patch %	Lines
src/psyclone/core/single_variable_access_info.py	97.29%	1 Missing ⚠️
...clone/domain/lfric/lfric_extract_driver_creator.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2825      +/-   ##
==========================================
- Coverage   99.88%   99.88%   -0.01%     
==========================================
  Files         357      357              
  Lines       49823    49896      +73     
==========================================
+ Hits        49767    49839      +72     
- Misses         56       57       +1     

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

[arporter]

This PR enhances the functionality of VariablesAccessInfo by adding reference_accesses methods to CodeBlock, Literal and ScopingNode. This then allows it to be used in KernelModuleInlineTrans() in place of the custom search for symbols that used to be performed there. We can probably use this functionality elsewhere but I've not investigated that here.

Ready for review. One for @hiker or @sergisiso.

[arporter]

(I've triggered the integration tests.)

[arporter]

Taking this back as both the NEMOv4 and LFRic integration tests failed :-(

[arporter]

LFRic ('with all transformations') test fails with:

algorithm/runtime_constants/physical_op_constants_mod_psy.f90:253:10:
41632

41633
  253 |       IF (psyclone_internal_cmp(geometry, geometry_planar)) THEN
41634
      |          1
41635
Error: Function 'psyclone_internal_cmp' at (1) has no IMPLICIT type

and the NEMO v.4 OpenACC kernels test fails at the final link stage:

NEMO/cfgs/SPITZ12_openacc_kernels/BLD/lib/lib__fcm__nemo.a(obs_oper.o): in function `obs_oper_obs_max_fpsize_':
14928
NEMO/cfgs/SPITZ12_openacc_kernels/WORK/obs_oper.f90:905: undefined reference to `obs_deg2dist_'
14929
NEMO/cfgs/SPITZ12_openacc_kernels/BLD/lib/lib__fcm__nemo.a(obs_oper.o): in function `obs_oper_obs_avg_h2d_init_':
14930
NEMO/cfgs/SPITZ12_openacc_kernels/WORK/obs_oper.f90:955: undefined reference to `obs_avg_h2d_rad_'
14931
NEMO/cfgs/SPITZ12_openacc_kernels/WORK/obs_oper.f90:959: undefined reference to `obs_avg_h2d_rec_'

I think the latter often means we've put OpenACC around a function call but the function itself hasn't been marked-up for GPU compilation?

[arporter]

In LFRic, we have an in-lined kernel that calls our PSyclone-generated routine but that routine hasn't been inlined:

      REAL(KIND=r_def), dimension(ndf_w2b,3) :: rad_vector_xyz

      ipanel = INT(panel_id(map_pid(1)), i_def)
      IF (psyclone_internal_cmp(geometry, geometry_planar)) THEN
        x_vector = (/1.0_r_def, 0.0_r_def, 0.0_r_def/)
        y_vector = (/0.0_r_def, 1.0_r_def, 0.0_r_def/)

Presumably, the symbol isn't being spotted by the transformation?

[arporter]

The issue seems to be that Call.reference_accesses() does not record the routine name. There is a TODO back to #2271 suggesting we need a new type of access in AccessType allowing us to capture the fact that a symbol is required at compile time. I can do this but we'll need agreement on what to call it. Currently we have:

Some suggestions:

SYMBOLIC, STATIC, COMPILE_TIME

What do @hiker and @sergisiso think?

[sergisiso]

I don't like SYMBOLIC, as any other reference is also symbolic.

I could live with STATIC and COMPILE_TIME but I don't know if they are descriptive enough (and it is similar to some of the symbol interfaces which can make it confusing).

Why don't we simply call them: CALL / CALLED

Note that before we discussed a new AccessType that would apply for things that Fortran calls inquire/inquiry e.g. lbound(ref), size(ref, 2), ..., I would keep this separate and call these "ref" AccessType INQUIRE.

[hiker]

I don't like SYMBOLIC, as any other reference is also symbolic.

I could live with STATIC and COMPILE_TIME but I don't know if they are descriptive enough (and it is similar to some of the symbol interfaces which can make it confusing).

Why don't we simply call them: CALL / CALLED

Note that before we discussed a new AccessType that would apply for things that Fortran calls inquire/inquiry e.g. lbound(ref), size(ref, 2), ..., I would keep this separate and call these "ref" AccessType INQUIRE.

I like, CALL or CALLED, too. And the idea of AccessType.INQUIRE is great, too. Probably better than the current solution of a flag (since then after all we still don't know what exactly a read access indicates, while then we can distinguish that).

Given that we are discussing access modes: while I am aware that I have added it, I wonder if we should keep READINC as access pattern. It seems to be very LFRic-specific? From a dependency point of view ... do we even need READWRITE, could that just become two accesses, one read, one write? OK, that's out of scope ;)

[arporter]

GungHo now builds and runs (OpenACC) :-)

[arporter]

Integration tests were green this time. Really ready for review now.

[sergisiso]

Thanks for starting the work on this issue @arporter , it will be a great improvement if we solve/do better on this (I see #2271 is marked as "towards" could you clarify what will be missing?). That said I have a some concerns with the current implementation:

• The main one is that this PR classifies kinds, datatypesymbols, ... as READs. I think this will create incorrect code in places that we use the VAI: data movement directives, openmp infer_shared_attributes, extraction driver, ... as these will appear in the list of variables when they should not (and we probably miss tests properly checking this). You added exceptions for the extraction driver, but I think the same will be needed everywhere that uses VAI, which makes me think that instead of the exceptions it would be easier to mark them as AccessType.TYPE_INFO/INQUIRY in this PR already, as I believe it will solve/clean up a few things. It could also allow to remove the options and COLLECT-ARRAY-SHAPE-READS.
• I am not sure the UNKNOWN AccessType is considered as worst-case scenario in most cases, which probably means we could come up with cases the make anything using VAI fail.
• Finally, not for this PR but VAI/signatures do not deal well with nested scopes, I think we need some xfails/references to reference_accesses does not support nested scopes #2424 in the places that this happens.
  See more details inline

[sergisiso]

Maybe # Continue processing references in any index expression

As they are not all reads, e.g. there may be another CALL, even with and output argument which would be WRITE

[sergisiso]

Same in a similar comment below which was already here

[sergisiso]

I know in the past we didn't do it for __str__ but maybe now we can use the -> str typehint for the sake of helping IDEs.

[sergisiso]

Maybe just -> AccessType if this is a valid typehint?

[sergisiso]

The second semicolon is in the wrong place

[sergisiso]

I like this new version, but note that this comes with the signatures limitations e.g. don't work with nested scopes:

• if the variable has a unique name

subroutine test()
    do i = 1, 10 <- PSyIR declares "a" here
        a(i) = 1
    end do 
end subroutine

and will fails with the TransformationError below.

• if the variable has a name clash

integer :: a
subroutine test()
    do i = 1, 10 <- PSyIR declares "a" here
        a(i) = 1
    end do 
end subroutine

it will fails with the "is declared in the same module scope".

Both cases are wrong and the subroutine could be inlined. Granted that this won't happen often because it requires inlining subroutines that have previously been transformed with psyclone so that a symbol was declared in a inner scope. But maybe we should document - add an xfail to #2424

[sergisiso]

Revert psy to _ as it is unused

[sergisiso]

Is the comment outdated? Why is i_def here now but r_def should not?

[sergisiso]

Maybe one of the indices should be another type-bound call with more indices, to check that it compounds properly.

[sergisiso]

As mentioned above, I would like to know what happens with "sqrt" (or any call), it is a signature?, is it readwrite or unkonwn?

[sergisiso]

First, instead of being "not" and adding the not is_called here, maybe this should be "is_read_first" and keep all the AccessType logic inside the method? It would also match what the docstring here says.

But in addition, I checked if "is_write_first" considers the UNKNON AccessType as a worst-case scenario for the first access (as I wondered this in a previous comment) and it does not. As this method are currently implemented they mean "is guaranteed to be read/write first?" but I think what we need here is "is potentially read/write first?" (but I am unsure I have not used the CallTreeUtils myself yet)

Maybe this is the typical issue with boolean methods that can be unknow, as we could add the is_read/write_first(unknown_as=True/False) to make it explicit that also used in other places?

[arporter]

The trouble with adding AccessType.INQUIRY is that it does not allow us to distinguish between compile-time constants (e.g. precision symbols) and arguments to e.g. array-shape query calls. The former aren't needed when extracting kernels but the latter are. However, when looking at e.g. inlining, we care about both. Possibly we therefore need AccessType.COMPILE_TIME too?