(Towards #1366) add support for IntrinsicCall (allocate, deallocate...)

[arporter]

[arporter]

Getting there now. I need to add support for mold and then either think about deallocate or stop and get this ready for review.

[arporter]

This PR will require #1911 to go onto master as it needs the 'mold' support in fparser.

[arporter]

This is now done but is waiting on #1911 to update fparser (as it requires support for the 'mold' argument to allocate).

[sergisiso]

@arporter I merged the fparser PR, this should be unblocked now.

[codecov[bot]]

Codecov Report

Base: 99.84% // Head: 99.84% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (5774be6) compared to base (01c2a28). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #1907    +/-   ##
========================================
  Coverage   99.84%   99.84%            
========================================
  Files         302      306     +4     
  Lines       41853    42045   +192     
========================================
+ Hits        41787    41979   +192     
  Misses         66       66            

Impacted Files	Coverage Δ
.../psyir/transformations/hoist_local_arrays_trans.py	100.00% <ø> (ø)
...c/psyclone/domain/common/extract_driver_creator.py	100.00% <100.00%> (ø)
src/psyclone/psyir/backend/fortran.py	100.00% <100.00%> (ø)
src/psyclone/psyir/frontend/fparser2.py	100.00% <100.00%> (ø)
src/psyclone/psyir/nodes/__init__.py	100.00% <100.00%> (ø)
src/psyclone/psyir/nodes/call.py	100.00% <100.00%> (ø)
src/psyclone/psyir/nodes/intrinsic_call.py	100.00% <100.00%> (ø)
src/psyclone/psyir/symbols/__init__.py	100.00% <100.00%> (ø)
src/psyclone/psyir/symbols/intrinsic_symbol.py	100.00% <100.00%> (ø)
src/psyclone/tests/psyir/backend/fortran_test.py	100.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[arporter]

Ready for first review now. Probably one for @sergisiso or @rupertford.

[arporter]

Thanks for the review Sergi. I should have written the documentation first as that always helps me to get things straight in my head. I've now drafted some:

and the first thing that becomes apparent is that actually, 'random' could still be an Operator as it has no optional arguments. I think it is the presence of the optional arguments that makes it very difficult to represent something as an Operator (unless we have a different Operator for each valid combination of optional arguments). MAXVAL and MINVAL will also be a good fit for IntrinsicCall I think.

[arporter]

I can see that there is an argument for moving all intrinsics from Operation to IntrinsicCall - it would clean up the fact that we already have multiple Operation nodes for e.g. MAX because it can have a variable number of arguments. As you say, the parallelisation issues will probably require us to have a pure or elemental property for IntrinsicCall. Do we need to have a telco to discuss this with @rupertford?

[rupertford]

Happy to discuss but my preference is for intrinsics to all be in the same place. I don't like some intrinsics being operators and some being IntrinsicCall as it is confusing and could also complicate things if we want to reason about other languages. I guess there is an argument that we could boil down intrinsics to ones without optional arguments and then build new ones (or generate code) for more complicated cases, but that is not the road we've gone down and I think understanding Fortran intrinsics (and perhaps C intrinsics in the future) in PSyIR fits in with its main purpose (performance).

[arporter]

I've (somewhat unilaterally) decided that it would be best if all intrinsics became IntrinsicCalls and have created #1987 to do this. I've updated the documentation to say this.

[arporter]

This is ready for another look now.

[arporter]

Ready for another look now, CI permitting.

[sergisiso]

It seems like this closes #1366 even though it is marked as "towards" in the PR title. I have marked as resolved the issue on the changelog and will close the issue after merging. @arporter feel free to let me know and reopen the issue if I am missing something.