LFRicTypes creates orphan Symbols

[arporter]

Currently, LFRicTypes just creates DataSymbols and ContainerSymbols (primarily for precision and their imports) as and when it needs them. They are never added to a SymbolTable. This has then meant that we have to take special action in datatypes::ScalarType.__eq__ as often, two otherwise identical LFRicIntegerScalarTypes have different precision symbols (with the same name).

Clearly, the symbols that LFRicTypes creates should be added to a table. Quite how to manage this isn't clear.

Similarly, LFRicConstants.precision_for_type currently uses LFRicTypes and returns a Symbol that again is not in a table. Given that this method currently has to locally import LFRicTypes, I suspect that really it should just return a name (str) and let the caller create a suitable symbol in the correct table.

[sergisiso]

@arporter I hit this today while trying to build upstream LFRic with upstream PSyclone, the compilation fails with:

13:23:46 Compile lfric_xios_setup_mod_psy.f90
lfric_xios_setup_mod_psy.f90:15:22:

   15 |     integer(kind=i_def) :: cell
      |                      1
Error: Symbol 'i_def' at (1) has no IMPLICIT type

If I open the psyclone generated file, indeed "i_def" is never imported.

Should we defensively import all kinds from constants_mod until this is fixed?

[sergisiso]

If I go to a commit before #2633 was merged it works.

[arporter]

Now that you say this, I remember hitting it too (but had forgotten). I think the LFRic compiler settings might be such that importing something but never using it will be flagged as an error. It would be better if we could fix it properly.

[sergisiso]

Unfortunately it's the opposite, it is used but never imported. It is a bug on psyclone's generation.

[arporter]

I think I meant that if we import all possible precision symbols every time, then some might not be used. However, as discussed at the telco today, it seems unlikely that a compiler will complain about this. Since I dug into this recently and I would like to fix it properly, I'll take a look.

[arporter]

If I just run PSyclone on lfric_xios_setup_mod.x90 then the generated PSy layer does have:

  MODULE lfric_xios_setup_mod_psy
    USE constants_mod, ONLY: r_def, i_def
    USE field_mod, ONLY: field_type, field_proxy_type
    IMPLICIT NONE
    CONTAINS

Did you get the failure after applying an optimisation script @sergisiso?

[arporter]

If I apply the 'standard' global.py used by the MO then the generated PSy layer does still have the required USE statement.

[sergisiso]

It still fails for me, I use lfric_apps 2689 and lfric_core 50706 as suggested in the dependencies.sh, if I do:

cd build
spack load gcc@14 lfric-buildenv@2689%gcc
./local_build.py -a gungho_model -j 12

[arporter]

Ah, OK. I was just testing with lfric_core (which is where the file lives) @r50758. I've just tried with 'everything-everywhere-all-at-once' and that looked OK too. (With latest master of PSyclone.)

lfric_apps$ psyclone -api lfric -d ./components/ -oalg /dev/null -s ~/Projects/PSyclone/examples/lfric/scripts/everything_everywhere_all_at_once.py ./components/lfric-xios/source/lfric_xios_setup_mod.x90

[sergisiso]

Well, after removing the directories and trying again (the ./local_build.py -a gungho_model -t clean hanged for me), now it works

[arporter]

Yes, the clean target always hangs for me too - the rm that it does is missing a -f and some of the files are protected.

[hiker]

I think I meant that if we import all possible precision symbols every time, then some might not be used. However, as discussed at the telco today, it seems unlikely that a compiler will complain about this. Since I dug into this recently and I would like to fix it properly, I'll take a look.

I just tried:

...
subroutine test()
    use mymod, only : a,b
    implicit none
    print *,a
end subroutine test

ifort -warn all -warn errors -gen-interfaces nosource  -c ./test.f90 
./test.f90(7): remark #7712: This variable has not been used.   [B]
    use mymod, only : a,b
------------------------^

That is a remark, not a warning. So the return code of the compiler is 0.

gfortran  -Wall -c  test.f90 
test.f90:7:8:

    7 |     use mymod, only : a,b
      |        1
Warning: Unused module variable ‘b’ which has been explicitly imported at (1) [-Wunused-variable]

gfortran's -Wall triggers a warning, but this warning in LFRic is not set to turn into an error (as some warnings are).

Using a generic (not only) import works fine.

[arporter]

As implemented, LFRicTypes is a singleton - when its constructor is called the first time it populates a dict and then those values are used for subsequent calls. If I alter this so that it always constructs all of the symbols every time then I get an infinite recursion as the constructor sets up all of the classes every time and some of these require other calls to the constructor: https://github.com/stfc/PSyclone/blob/4297270ae65e190b246432768a7cba86357118ba/src/psyclone/domain/lfric/lfric_types.py#L118-L130 I could possible avoid this problem if I broke out the various _create calls into separate, public methods. Perhaps it would make sense to access these via the LFRicSymbolTable?

[arporter]

Note that it is only the _create_precision_from_const_module() that creates new symbols and thus needs access to a table.

[arporter]

Another part of the problem is that LFRic types creates everything, all in one go, while in general, we only create/add Symbols to a table as they are actually required. We don't want to add everything to a table just on the off chance. Having said that, the only Symbols created every time are the precision symbols and constants_mod so that they can be imported from it. I think this is actually OK (and we know from above that compilers are happy with this).

[arporter]

Since the types are inextricably linked to the precision symbols that they use, I've moved the required LFRicTypes functionality into the LFRicSymbolTable class instead. This has removed some duplication (of handling the creation of precision symbols and the associated module). Since we no longer have a singleton, we can (and do) get more than one instance of each type class. This makes comparing them slightly tricky so I've resorted to comparing by using type(obj).__name__.