Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Towards #2156) Changes to routine_copy and fortran backend for elemental and impure support #2566

Merged
merged 18 commits into from
May 22, 2024

Conversation

LonelyCat124
Copy link
Collaborator

A few small changes required to make the PSyIR layer (mostly the backends) not lose information on pure and elemental subroutines.

This might solve some of the NEMO issues reported, but only if the subroutines are contained within modules.

@LonelyCat124 LonelyCat124 requested review from sergisiso and arporter May 2, 2024 08:35
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (453bfa6) to head (2b8cc6d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2566   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         354      354           
  Lines       48330    48411   +81     
=======================================
+ Hits        48267    48348   +81     
  Misses         63       63           

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

@LonelyCat124
Copy link
Collaborator Author

I've had to change something in fparser2.py now as well - the default behaviour is now that the purity of a subroutine is None (i.e. unknown) unless it is specified. This is required as elemental implies pure unless specified, however an input of elemental without pure will result in impure elemental with the old implementation, and I believe unknown => pure.

We could also adapt this to have elemental set is_pure=True unless is_pure is already False, but I will leave that to the reviewer.

@arporter this is ready for review now - may be helpful to have before Nemo party but I'm not sure if its sufficient to fix their issues.

@LonelyCat124
Copy link
Collaborator Author

Also I realise that this doesn't technically fix the stuff for the issue 2156 but just andy's comments within.

@LonelyCat124
Copy link
Collaborator Author

@arporter @sergisiso Should we also ask to test this branch with NEMO before the NEMO party?

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Aidan, this looks good to me.
Just a couple of very minor things to tidy up. I can't yet run the integration tests as the python modules on the runner need fixing so that will have to wait until next time.
Ref guide still builds fine.

src/psyclone/psyir/backend/fortran.py Show resolved Hide resolved
src/psyclone/psyir/symbols/routinesymbol.py Outdated Show resolved Hide resolved
@LonelyCat124
Copy link
Collaborator Author

@arporter changes done so should be ready for another look once the runner on glados is up and running again.

@sergisiso
Copy link
Collaborator

Thanks @LonelyCat124 to implement this, looking at NEMO upstream it seems that they all are elemental functions, I think it will work with your implementation but it would be good to add some tests with functions to make sure it all goes right(frontend-attribute-backend).
The signatures in nemo are:
ELEMENTAL FUNCTION SIGN_SCALAR( val1, val2 )
and there is one:
impure ELEMENTAL FUNCTION pres_temp_sclr( val)

@LonelyCat124
Copy link
Collaborator Author

LonelyCat124 commented May 8, 2024

Thanks @LonelyCat124 to implement this, looking at NEMO upstream it seems that they all are elemental functions, I think it will work with your implementation but it would be good to add some tests with functions to make sure it all goes right(frontend-attribute-backend). The signatures in nemo are: ELEMENTAL FUNCTION SIGN_SCALAR( val1, val2 ) and there is one: impure ELEMENTAL FUNCTION pres_temp_sclr( val)

I'm guessing there's some return type on these functions too? It works if I use real elemental function but I get a codeblock from something else if its just elemental function x

Edit: Never mind, I made it work without the return type declared on the function now.

Tests added - ready for another look

@sergisiso
Copy link
Collaborator

In nemo functions the type is in the declarations (with the same name as the function), not the function line:

   ELEMENTAL FUNCTION SIGN_SCALAR( pa, pb )
      !!-----------------------------------------------------------------------
      !!                  ***  FUNCTION SIGN_SCALAR  ***
      !!
      !! ** Purpose : overwrite f95 behaviour of intrinsinc sign function
      !!-----------------------------------------------------------------------
      REAL(wp), INTENT(in) :: pa,pb          ! input
      REAL(wp)             :: SIGN_SCALAR    ! result
      !!-----------------------------------------------------------------------
      IF ( pb >= 0._wp ) THEN   ;   SIGN_SCALAR =  ABS(pa)
      ELSE                      ;   SIGN_SCALAR = -ABS(pa)
      ENDIF
   END FUNCTION SIGN_SCALAR

@LonelyCat124
Copy link
Collaborator Author

Yeah - I put that in the tests and it works. PSyclone does just create a codeblock if I have real elemental function but I don't know why that is, I assumed it was a generic PSyclone thing and not related to this PR.

@LonelyCat124
Copy link
Collaborator Author

LonelyCat124 commented May 8, 2024

Also - should I do something on xfailing tests that makes the test fail if the xfail condition isn't met? I feel like those tests should either xfail or fail - if they succeed we should have a proper test for them (I just realised while playing around with #2201 that if I make the xfail test work it doesn't report anything wrong) - adding an else in the test fails coverage.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All requested changes have been made (thanks). I'll fire off the integration tests once more.

Actually, I can't run the integration tests until #2571 is on.

@arporter arporter added the Blocked An issue/PR that is blocked by one or more issues/PRs. label May 9, 2024
@arporter arporter removed the Blocked An issue/PR that is blocked by one or more issues/PRs. label May 10, 2024
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Aidan.
A comment will really help me understand what your new addition to routine.py is doing. Aside from that there's just a bit of tidying to do.
I've fired off the integration tests as they are now working again for NEMO and compilation.
Ref. guide still builds fine.

src/psyclone/tests/psyir/backend/fortran_routine_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyir/backend/fortran_routine_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyir/backend/fortran_routine_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyir/backend/fortran_routine_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyir/backend/fortran_routine_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyir/backend/fortran_routine_test.py Outdated Show resolved Hide resolved
src/psyclone/psyir/nodes/routine.py Show resolved Hide resolved
src/psyclone/psyir/nodes/routine.py Outdated Show resolved Hide resolved
@arporter
Copy link
Member

Just a note to record that the NEMO and compilation integration tests were fine for this PR. (LFRic tests are currently not working.)

@LonelyCat124
Copy link
Collaborator Author

Ready for another look now, let me know if you think the comment is sufficient.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All requested changes have been made. Integration tests are all green.
Will proceed to merge.

src/psyclone/psyir/nodes/routine.py Show resolved Hide resolved
src/psyclone/psyir/nodes/routine.py Outdated Show resolved Hide resolved
src/psyclone/psyir/nodes/routine.py Show resolved Hide resolved
@arporter arporter merged commit d334f74 into master May 22, 2024
11 checks passed
@arporter arporter deleted the 2156_elemental_subroutines branch May 22, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants