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

Inline functions (including multi-line and non-elemental functions) #378

Merged
merged 10 commits into from
Oct 10, 2024

Conversation

MichaelSt98
Copy link
Collaborator

Extending and improving Inline utilities as discussed in #377

Copy link

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/378/index.html

@MichaelSt98 MichaelSt98 marked this pull request as draft September 23, 2024 15:13
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 98.41270% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.51%. Comparing base (3834903) to head (53cc3fa).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
loki/transformations/inline.py 97.61% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
- Coverage   95.53%   95.51%   -0.03%     
==========================================
  Files         186      186              
  Lines       38974    39140     +166     
==========================================
+ Hits        37233    37383     +150     
- Misses       1741     1757      +16     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 95.49% <98.41%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@MichaelSt98 MichaelSt98 marked this pull request as ready for review September 24, 2024 15:56
@MichaelSt98
Copy link
Collaborator Author

The whole thing got a bit out of hand again and turned out to be a rabbit hole ...

Currently (or rather with this PR), nested functions are handled via an iterative approach. We could try to solve this recursively instead. However, I don't think this would be straight-forward and questionable whether this is beneficial. Anyway, I am open for discussing that and other implementation details.

Moreover, it would be possible to check whether functions are single-line (or could be reduced to a single line) and inline those at an expression level.

Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

Looks great, and is very, very useful! Many thanks, this one has been on the todo list for a while! 🙏 🙏 🙏 🙏 🙏 🙏 GTG :shipit:

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Very nicely done, this is a really fantastic addition and nicely executed!

A bunch of nitpick comments but no showstoppers.

.pylintrc Outdated
@@ -87,6 +87,7 @@ disable=W0511, # Disable TODO warnings
pointless-string-statement,
attribute-defined-outside-init,
protected-access,
too-many-positional-arguments,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has also been fixed on the main branch, so we might want to remove the commit from this branch in a rebase.

loki/transformations/inline.py Show resolved Hide resolved
loki/transformations/inline.py Show resolved Hide resolved
loki/transformations/inline.py Outdated Show resolved Hide resolved
loki/transformations/inline.py Show resolved Hide resolved
loki/transformations/inline.py Outdated Show resolved Hide resolved
@@ -367,6 +465,81 @@ def test_inline_member_routines(tmp_path, frontend):
assert (b == [3., 3., 3.]).all()


@pytest.mark.parametrize('frontend', available_frontends(skip={
OMNI: "OMNI can't parse the code"}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? I can't see anything that would usually upset OMNI, and a quick parse with F_Front was successful for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you execute

from loki import Subroutine, fgen, OMNI, FP
fcode = """
subroutine member_functions
    implicit none
    integer :: i
    real(kind=8) :: a(3)
    contains
    function add_to_a(b, n)
      integer, intent(in) :: n
      real(kind=8), intent(in) :: b(n)
      real(kind=8) :: add_to_a(n)

      do i = 1, n
        add_to_a(i) = a(i) + b(i)
      end do
    end function
end subroutine member_functions
    """
routine = Subroutine.from_source(fcode, frontend=OMNI)
print(fgen(routine))
SUBROUTINE member_functions ()
  IMPLICIT NONE
  INTEGER :: i
  REAL(KIND=8) :: a(3)
  CONTAINS
  FUNCTION add_to_a (b, n)
    IMPLICIT NONE
    INTEGER, INTENT(IN) :: n
    REAL(KIND=8), INTENT(IN) :: b(n)
    ! HERE: MISSING DIMENSION !!!
    REAL(KIND=8) :: add_to_a
    DO i=1,n
      add_to_a(i) = a(i) + b(i)
    END DO
  END FUNCTION add_to_a
END SUBROUTINE member_functions

Same thing for real(kind=8), dimension(n) :: add_to_a. This doesn't happen for FP and OFP.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This does sound like a bug in the Loki frontend, translating the OMNI AST to Loki IR, not a problem with OMNI.
Give me a sec, I'll have a look...

Copy link
Collaborator

Choose a reason for hiding this comment

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

#391 should fix the problem

loki/transformations/tests/test_inline.py Show resolved Hide resolved
@reuterbal reuterbal force-pushed the nams-inline-elemental-functions branch from 3be60e1 to 53cc3fa Compare October 9, 2024 08:30
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

I have rebased and removed the pylintrc commit. Ready from my side now, many thanks again!

@reuterbal reuterbal added the ready for merge This PR has been approved and is ready to be merged label Oct 9, 2024
@reuterbal reuterbal merged commit 8cd5702 into main Oct 10, 2024
13 checks passed
@reuterbal reuterbal deleted the nams-inline-elemental-functions branch October 10, 2024 11:36
@reuterbal reuterbal linked an issue Oct 14, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function inlining
3 participants