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.

Sign up for GitHub

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

Jump to bottom

Allow inlining of Statement Functions #345

Merged

reuterbal merged 7 commits into main from nams-inline-stmt-funcs

Jul 24, 2024

Collaborator

MichaelSt98 commented Jul 18, 2024

OFP excluded from test because of some problems with Statement Functions I haven't investigated further yet
if the order or statement function declaration is "wrong" this won't work as the statement function would be interpreted as array access ... but this is a problem that has nothing to do with the inlining itself

From

subroutine stmt_func(arr, ret)
    implicit none
    real, intent(in) :: arr(:)
    real, intent(inout) :: ret(:)
    real :: ret2
    real, parameter :: rtt = 1.0
    real :: PTARE
    real :: FOEDELTA
    FOEDELTA ( PTARE ) = PTARE + 1.0
    real :: FOEEW
    FOEEW ( PTARE ) = PTARE + FOEDELTA(PTARE)

    ret = foeew(arr) 
    ret2 = foedelta(3.0)
end subroutine stmt_func

to

SUBROUTINE stmt_func (arr, ret)
  IMPLICIT NONE
  REAL, INTENT(IN) :: arr(:)
  REAL, INTENT(INOUT) :: ret(:)
  REAL :: ret2
  REAL, PARAMETER :: rtt = 1.0
  
  ret = arr + arr + 1.0
  ret2 = 3.0 + 1.0
END SUBROUTINE stmt_func

github-actions bot commented Jul 18, 2024

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

codecov bot commented Jul 18, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.31%. Comparing base (78e7605) to head (a29f729).

Files	Patch %	Lines
loki/transformations/inline.py	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   95.29%   95.31%   +0.02%     
==========================================
  Files         170      170              
  Lines       36414    36467      +53     
==========================================
+ Hits        34701    34760      +59     
+ Misses       1713     1707       -6

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`95.30% <98.30%> (+0.02%)`	⬆️

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.

Collaborator Author

MichaelSt98 commented Jul 18, 2024

Maybe it would be better to not replicate the functionality of recursive_expression_map_update(expr_map, max_iterations=10): and instead introduce a new argument mapper to allow using the InlineSubstitutionMapper instead of the SubstitutionMapper...
What do you think?

MichaelSt98 requested review from reuterbal and mlange05

July 18, 2024 15:59

reuterbal requested changes

View reviewed changes

Collaborator

reuterbal left a comment

Thanks, this looks great. Small remarks on improving the implementation and I agree with your suggestion to merge the expression map update.

loki/transformations/tests/test_inline.py Outdated

+                  real, intent(inout) :: ret(:)
+                  real :: ret2
+                  real, parameter :: rtt = 1.0
+                  {stmt_decls_code if stmt_decls else ''}

Collaborator

reuterbal Jul 19, 2024

We would usually have a header include instead, no?

Suggested change

      
                {stmt_decls_code if stmt_decls else ''}
          
                {stmt_decls_code if stmt_decls else '#include "fcttre.func.h"'}

Collaborator Author

MichaelSt98 Jul 22, 2024

That is correct. However, it does not change anything here.

loki/transformations/inline.py Outdated

+              def inline_statement_functions(routine):
+                  """
+                  Replaces `InlineCall` expression to statement functions with the

Collaborator

reuterbal Jul 19, 2024

Suggested change

      
                Replaces `InlineCall` expression to statement functions with the
          
                Replaces :any:`InlineCall` expression to statement functions with the

loki/transformations/inline.py Outdated

		@@ -339,6 +360,89 @@ def inline_elemental_functions(routine):
		routine.spec = Transformer(import_map).visit(routine.spec)


		def recursive_inline_expression_map_update(expr_map, max_iterations=10):

Collaborator

reuterbal Jul 19, 2024

I agree with your remark, it's probably best to avoid the code duplication and instead introduce an additional argument mapper_cls=SubstituteExpressionsMapper to the recursive_expression_map_update function.

loki/transformations/inline.py

Comment on lines +132 to +134

+                      # Inline Statement Functions
+                      if self.inline_stmt_funcs:
+                          inline_statement_functions(routine)

Collaborator

reuterbal Jul 19, 2024

This code path is currently untested. Can we add a test where InlineTransformation is called to apply the statement function inlining?

loki/transformations/inline.py Outdated

Comment on lines 201 to 204

+                          function = expr.routine
+                          v_result = expr.routine.variable
+                          # Substitute all arguments through the elemental body
+                          arg_map = dict(zip(function.arguments, expr.parameters))

Collaborator

reuterbal Jul 19, 2024

Just wondering: Wouldn't arg_map = dict(expr.arg_iter()) work here, and also below for "regular" inline calls? That should also take care of keyword arguments.
(Cc @mlange05)

loki/transformations/inline.py Outdated

Comment on lines 417 to 419

+                      if call.procedure_type is BasicType.DEFERRED:
+                          continue
+                      if call.procedure_type.is_function and isinstance(call.routine, StatementFunction):

Collaborator

reuterbal Jul 19, 2024

Just a little hint to avoid repeated lookup of type info:

Suggested change

      
                    if call.procedure_type is BasicType.DEFERRED:
          
                        continue
          
                    if call.procedure_type.is_function and isinstance(call.routine, StatementFunction):
          
                    proc_type = call.procedure_type
          
                    if proc_type is BasicType.DEFERRED:
          
                        continue
          
                    if proc_type.is_function and isinstance(proc_type.procedure, StatementFunction):

loki/transformations/inline.py Outdated

Comment on lines 428 to 443

+                  stmt_arguments = set()
+                  stmt_func_map = {}
+                  for stmt_func_decl in stmt_func_decls:
+                      for arg in stmt_func_decl.arguments:
+                          stmt_arguments.add(arg)
+                      stmt_func_map[stmt_func_decl] = None
+                  # remove the statement function declarations
+                  routine.spec = Transformer(stmt_func_map).visit(routine.spec)
+                  # remove the declarations of the statement function arguments
+                  decls = FindNodes(VariableDeclaration).visit(routine.spec)
+                  decls = tuple(d for d in decls if any(isinstance(s, sym.ProcedureSymbol) or s in stmt_arguments for s in d.symbols))
+                  decl_map = {}
+                  for decl in decls:
+                      decl_map[decl] = None
+                  routine.spec = Transformer(decl_map).visit(routine.spec)

Collaborator

reuterbal Jul 19, 2024

This boils down to removing a bunch of variable declarations and everything in stmt_func_decls, so that should be possible in a single Transformer pass. Maybe something along these lines works?

vars_to_remove = {stmt_func.variable.name.lower() for stmt_func in stmt_func_decls}
vars_to_remove += {arg.name.lower() for stmt_func in stmt_func_decls for arg in stmt_func.arguments}
spec_map = {stmt_func: None for stmt_func in stmt_func_decls}
for decl in routine.declarations:
    if any(var in vars_to_remove for var in decl.symbols):
        symbols = tuple(var for var in decl.symbols if var not in vars_to_remove)
        if symbols:
            decl._update(symbols=symbols)
        else:
            spec_map[decl] = None
routine.spec = Transformer(spec_map).visit(routine.spec)

MichaelSt98 requested a review from reuterbal

July 22, 2024 12:58

reuterbal added the rebase required label

reuterbal reviewed

View reviewed changes

Collaborator

reuterbal left a comment

Thanks, this looks great now!
Due to other merged PRs we have a conflict now, would you mind rebasing this over latest main? I hope this will then also trigger the tests again.

loki/transformations/inline.py Outdated

+                      # if it is an inline call to a Statement Function
+                      if isinstance(expr.routine, StatementFunction):
+                          function = expr.routine
+                          v_result = expr.routine.variable

Collaborator

reuterbal Jul 22, 2024

This looks unused?

MichaelSt98 added 7 commits

July 22, 2024 15:58


          extend InlineTransformation and utilities to allow inlining of statem…

1065e1b

…ent functions


          improve test for inline statement functions and make inlining of stat…

ad82b64

…ement functions to be default False


          introduce 'mapper_cls' as argument for 'recursive_expression_map_update'

661c426


          some refactoring for inlining statement functions

553ee68


          extend tests to include test of inlining a statement function via the…

56d4931

… InlineTransformation


          use expr.arg_iter() instead of manual zipping inline call arguments a…

0f91045

…nd corresponding function arguments


          remove unused variable

a29f729

MichaelSt98 force-pushed the nams-inline-stmt-funcs branch from 838b353 to a29f729 Compare

July 22, 2024 14:01

MichaelSt98 requested a review from reuterbal

July 23, 2024 07:37

reuterbal approved these changes

View reviewed changes

reuterbal added ready for merge and removed rebase required labels

mlange05 approved these changes

View reviewed changes

Collaborator

mlange05 left a comment

Looks fantastic! Neatly implemented, neatly tested and very powerful! Very nice! GTG from me.

loki/transformations/utilities.py

@@ @@ -468,7 +468,7 @@ def replace_selected_kind(routine): @@
                           routine.spec.prepend(imprt)
-              def recursive_expression_map_update(expr_map, max_iterations=10):
+              def recursive_expression_map_update(expr_map, max_iterations=10, mapper_cls=SubstituteExpressionsMapper):

Collaborator

mlange05 Jul 24, 2024

No action: Very neat! 👏

reuterbal merged commit 7ca9dd8 into main

12 checks passed

reuterbal deleted the nams-inline-stmt-funcs branch

July 24, 2024 07:21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for merge