Codeblock for Module when declaring a procedure pointer

[hiker]

The LFRic source code components/science/source/kernel/geometry/chi_transform_mod.F90 contains several procedure pointer, e.g.:

 PROCEDURE(chir2xyz_interface), PROTECTED, POINTER :: chir2xyz => null()

This causes the whole module to become a CodeBlock, i.e.:

 FileContainer[]
    CodeBlock[[<class 'fparser.two.Fortran2003.Module'>]]

[LonelyCat124]

'm not sure this is a bug - this is expected behaviour and should be an enhancement tag imo. The fparser2 frontend doesn't support the Procedure_Declaration_Stmt node from fparser, so fails at line 2677 resulting in the module becoming a code block because it has unsupported symbol types in its symbol declaration.

This would need to be added into the frontend, either to be considered a generic symbol (?) to an UnsupportedFortranType (maybe?) or potentially a RoutineSymbol to an UnsupportedFortranType, though I'm not sure how pointer is supported for that, someone with more insight on symbol_table/symbols would be needed.

I think once the latter is understood adding it into fparser2.py is probably a fairly small job.

Standalone test for PSyclone

def test_procedure_pointer(fortran_reader):
    code = '''module mymod
    PROCEDURE(chir2xyz_interface), PROTECTED, POINTER :: chir2xyz => null() 
 end module'''
    psyir= fortran_reader.psyir_from_source(code)
    print(psyir.children[0])
    assert False

Edit: I'm also a bit concerned how inlinetrans would need to interact with these symbols also...

[hiker]

I'm not sure this is a bug - this is expected behaviour and should be an enhancement tag imo.

I would expect that one single statement/declaration that is unsupported in a module itself is turned into an Unsupported... class, or a CodeBlock - but not that the whole module, i.e. that all the other lines that are parsed just fine, end up as a code block as well. Ideally we would enhance PSyIR to represent it (which is indeed an enhancement), but it would also be sufficient initially to just be able to process files with a procedure pointer without getting a codeblock which raises exceptions and then basically aborts.

[LonelyCat124]

That would be true if the statement was a PSyIR statement (e.g. a line of code), but symbol declarations (and things that go into symbol tables) are not PSyIR nodes, they are members of the symbol table.

What happens during parsing this section of the code is we find something unhandled and give up (as the fparser frontend always does with any unknown object) and the containing element becomes a CodeBlock - since in this case the closest PSyIR node is the containing module, the module is just turned into a CodeBlock instead of a single statement. If there are other types of symbol we don't support we'd see the same behaviour.

We could create a Symbol to an UnsupportedFortranType for things we don't know about, but I'm not sure what else this might break (e.g. what happens if you have a Call to this procedure pointer - does Call to a unknown symbol result in a failure? Probably it would)

What are the exceptions and errors that you see, since the test above just results in

FileContainer[
CodeBlock[1 nodes]]

but I get no exceptions - if we have errors and aborts then we can try to fix whatever causes those until we can support this.

[hiker]

I've just assigned this to me, I will fix it the same way I've fixed namelist statements (see #2463). The parsing is fixed in a few lines:

...
            elif isinstance(node, Fortran2003.Procedure_Declaration_Stmt):
                parent.symbol_table.new_symbol(
                    root_name="_PSYCLONE_INTERNAL_PROC_POINTER",
                    symbol_type=DataSymbol,
                    datatype=UnsupportedFortranType(str(node)))

and all tests are still passing. This is of course not sufficient to properly handle the pointer (which can be done as an enhancement later), but sufficient to fix the bug that a whole module is turned into a code block unnecessary.

A call to that name will force PSyclone to (incorrectly) create a new RoutineSymbol, but again, fixing this can be done later (and it does not create duplicated symbols in the created code). Now the output (after adding a call) is:

FileContainer[]
    Container[mymod]
        Routine[name:'bla']
            0: Call[name='chir2xyz']

and

module mymod
  implicit none
  PROCEDURE(chir2xyz_interface), PROTECTED, POINTER :: chir2xyz => null()
  public

  contains
  subroutine bla()

    call chir2xyz()

  end subroutine bla

end module mymod

Good enough for me :)

FWIW, the errors happen in my driver creation when I am following the call tree and happen to hit this unexpected codeblock instead of a container object.

[LonelyCat124]

I guess we end up with a call to an unresolved routine symbol which is probably fine. I think a comment in fparser2.py explaining what this is doing and why.

I do think that

 root_name="_PSYCLONE_INTERNAL_PROC_POINTER",

should be

root_name = node.name # slash whatever it is to get the symbol name

and it should not be allowed to be renamed (allow_renaming=False) since we are actually adding a specific thing into the symbol table which we know the name of, even if we don't know the detail of it, and we shouldn't allow another symbol with that name into that scope.

Edit:
The downside to that suggestion is that because its a DataSymbol we might fail when coming across something like:

Subroutine mysub()
  PROCEDURE(chir2xyz_interface), PROTECTED, POINTER :: chir2xyz => null()

  call chir2xyz()
End Subroutine

since a call expects either a Symbol or a RoutineSymbol from the symbol table - but we could instead expand this to allow DataSymbol to an UnsupportedFortranType - however I'd need to see a test to see exactly how this behaves. I'm not keen on adding a symbol with an arbitrary name when we know the name of the symbol explicitly from fparser, but maybe it ends up being more trouble than it is worth.

[arporter]

I'm a bit worried about this change. You may have already handled this but you'll need to be careful that we don't generate an accessibility statement for a RoutineSymbol. Also, "A call to that name will force PSyclone to (incorrectly) create a new RoutineSymbol," sounds dangerous. We don't want a quick change here to start propagating into us generating incorrect PSyIR.

[hiker]

There is no change or patch or anything at this stage, I just copied and quickly modified what is happening in case of a namelist statement, which is based on what Andy added to support named common blocks in save statements. Only to show that it is really easy to avoid the bug of getting a code block for a whole file.

I am not claiming that this is the solution, and I am aware that our ability with unknown statements might have increased since I worked on it last.

FWIW, my example solution-hack would not introduce any larger risk than any undeclared call of a subroutine - the "dangerous" code that adds a generic routine symbols is already there, I have not added it.

The downside to that suggestion is that because its a DataSymbol we might fail when coming across something like:

Subroutine mysub()
PROCEDURE(chir2xyz_interface), PROTECTED, POINTER :: chir2xyz => null()

call chir2xyz()
End Subroutine

It doesn't. And tbh - even when, anything is better than converting the whole file into a code block, because in the latter case we can extract absolutely nothing from that file, as opposed to not be able to handle one symbol properly.

If you don't agree with this, please just close this ticket as 'not a bug', I am ok with it, I really don't want to waste my time arguing about this. There is a certain risk that this will then be re-opened by the UM-physics to GPU project by the people who are working on ukca, since my understanding is that PSyclone will be expected to handle these codes, and I am reasonable certain that PSyclone will be struggling to port a code-block to run on GPU ;) Of course, procedure pointers will be a pita to automatically convert to GPU ;)

[LonelyCat124]

I think we probably need to decide how PSyclone handles POINTERs (which I don't think it does yet? @sergisiso @arporter correct me if i'm wrong) and then enable that on RoutineSymbols and then this is handled properly and the latter is not a lot more work after the former, which we need anyway I think for various points in all of NG-ARCH.

Once we have the RoutineSymbol pointer in scope the code that adds the unknown RoutineSymbol won't be reached - i think thats just the existing code that handles things we don't know about (for whatever reason, might just be linked at linking time etc.).

As originally suggested I think we should swap the tag on this to enhancement (and probably NG-ARCH as well) and either have a discussion on how to handle POINTER or use whats already there to solve this.

[sergisiso]

I am not sure the issue is the pointer.

• I added support for pointer assignment in a branch and will put a PR soon, but these are to avoid CodeBlocks inside the subroutine, not what happens here.
• We put declarations with pointer in UnsupportedFortranTypes, we could improve this but it will still be a symbol.

The Codeblock here must come from the PROCEDURE or the PROTECTED attributes

[LonelyCat124]

The codeblock here comes from procedure for sure, its more how do we add support for it into PSyclone (procedure + pointer i think both, having one doesn't make sense). I guess if it would still end up as a DataSymbol with an UnsupportedFortranType its the same as the solution here - but I think ideally we would want it to be a RoutineSymbol so the creation of the call node would find that RoutineSymbol instead of creating a new one.

Edit:
Equally I don't know if adding an is_pointer attribute to RoutineSymbols is a complex task - i think we maybe only need to change any code if its True but assignments might be odd (since Reference to RoutineSymbol would have to be allowed). The alternative is allowing Calls to any Symbol (or possible just Routine or Data Symbols)

[sergisiso]

Ok, so then, unrelated to this PR, the reason I typically stopped adding support for pointers is that they will break the assumptions that there are no aliasing between symbols memory. The dependency analysis and some of the transformations rely on this and will be very complicated to properly support them.

The reason I was ok with them being UnsupportedFortranTypes is that in theory they have the same lack of guarantees, and if the dependency analysis or a transformation goes over them is a bug and needs to be fixed. If we introduced a special type for them we would need to go to all these places and add exceptions for the new pointer types in addition to the UFT.

[LonelyCat124]

Ok - so it seems the recommended solution to this for now is:

1. Take Joerg's modification to fparser2 to handle the declaration - adding a DataSymbol to an UnsupportedFortranType
2. Modify Call nodes to allow the symbol to be any Symbol instead of specifically a RoutineSymbol - probably there will need to be some m,opdifications elsewhere in the code to handle these Call nodes too possibly
3. When creating a call in fparser2.py, allow any symbol in the scope to be used on the call if it has the correct name (or we could restrict it here to be routine symbol or a symbol with an unsupported type - that might be better)

[hiker]

I've removed the 'bug' category, and removed myself as assignee. I am looking forward for this to get fixed^h^h^h^h^henhanced.

[arporter]

Only to show that it is really easy to avoid the bug of getting a code block for a whole file.

Just to return to this (I've been a bit snowed under). Getting a CodeBlock is not a bug - it is the way things are supposed to work. The 'advantage' of a CodeBlock is that it means we reproduce the original code unchanged and are therefore less likely to get something wrong. As soon as we start trying to do better we quickly expose ourselves to processing more code and that can throw up problems. I'm just being cautious!