Inline elemental functions: skip calls with args being array (slices) #401

MichaelSt98 · 2024-10-14T15:35:46Z

We have a problem for things like:

elemental function elem(x, y)
    real :: x, y, elem, tmp
    temp = y * 2.0
    elem = x + tmp
end function elem

real :: x_scalar, b_scalar, res_scalar
real :: x_arr(10). y_arr(10), res_arr(10)
real :: x_arr_2(12). y_arr_2(12), res_arr_2(12)

res_scalar = elem(x_scalar, y_scalar)
res_arr = elem(x_arr, y_arr)
res_arr = elem(x_arr, y_scalar)
res_arr = elem(y_scalar, x_arr)
res_arr_2 = elem(x_arr_2, y_arr_2)
...

As the fix is not straight-forward/trivial, this PR makes the inline to skip the problematic calls (for now). Thus, this is not a fix to make those problematic calls being inlined correctly!

A more detailed description of the problem: #400

reuterbal

Thanks for this!

For readability, we could make the checks a bit shorter (suggestions in inline comments).

The test is blowing a bit out of proportion this way and I would prefer splitting the completely separate "array" case into a separate test. I left a suggestion how you can avoid code duplication by moving the module into a joint fixture.

Finally, this requires a rebase due to the refactoring of inline and extract packages.

reuterbal · 2024-10-14T16:23:39Z

loki/transformations/tests/test_inline.py

+    fcode_arr = """
+subroutine transform_inline_elemental_functions_extended_array(v1, v2, v3, len)
+  use iso_fortran_env, only: real64
+  use multiply_extended_mod, only: multiply, multiply_single_line, add
+  integer, intent(in) :: len  
+  real(kind=real64), intent(in) :: v1(len)
+  real(kind=real64), intent(inout) :: v2(len), v3(len)
+  real(kind=real64), parameter :: param1 = 100.
+  integer, parameter :: arr_index = 1
+
+  v2 = multiply(v1(:), 6._real64) + multiply_single_line(v1(:), 3._real64)
+  v3 = add(param1, 200._real64) + add(v1, 150._real64) + multiply(v1(arr_index), v2(1))
+end subroutine transform_inline_elemental_functions_extended_array
 """


Let's make the array case a separate test, please.

You can still avoid replicating the module code etc using the following pattern:

@pytest.fixture(name='multiply_extended_mod', params=available_frontends()) def fixture_multiply_extended_mod(request, tmp_path): fcode_module = """ ... """ frontend = request.param module = Module.from_source(fcode_module, frontend=frontend, xmods=[tmp_path]) return module, frontend def test_transform_inline_elemental_functions_extended_scalar(multiply_extended_mod, tmp_path): module, frontend = multiply_extended_mod fcode = "..." routine = Subroutine.from_source(fcode, frontend=frontend, definitions=[module], xmods=[tmp_path]) ... def test_transform_inline_elemental_functions_extended_arr(multiply_extended_mod, tmp_path): module, frontend = multiply_extended_mod fcode_arr = "..." routine = Subroutine.from_source(fcode_arr, frontend=frontend, definitions=[module], xmods=[tmp_path]) ...

reuterbal · 2024-10-14T16:38:38Z

loki/transformations/inline.py

+                    return
+            if (expr.procedure_type.is_function and expr.procedure_type.is_elemental):
+                if any(is_array(val) for val in expr.arg_map.values()):
+                    warning(f'Call to elemental function \'{expr.routine.name}\' with array arguments.'


You can avoid escaping if you mix quotation marks:

Suggested change

warning(f'Call to elemental function \'{expr.routine.name}\' with array arguments.'

warning(f"Call to elemental function '{expr.routine.name}' with array arguments."

reuterbal · 2024-10-14T16:41:10Z

loki/transformations/inline.py

@@ -389,6 +393,19 @@ def __init__(self, query, recurse_query=None, inline_elementals_only=False,
        def map_inline_call(self, expr, *args, **kwargs):
            if not self.visit(expr, *args, **kwargs):
                return
+            if expr.procedure_type is BasicType.DEFERRED or isinstance(expr.routine, StatementFunction):
+                return


We could add here a bailout for non-functions and save a few checks further down:

Suggested change

return

return

if not expr.procedure_type.is_function:

return

Hmm this ends up with errors like:

AttributeError: 'BasicType' object has no attribute 'is_function'

You should add this below the checks in ll. 396-397

reuterbal · 2024-10-14T16:41:44Z

loki/transformations/inline.py

+            if self.inline_elementals_only:
+                if not (expr.procedure_type.is_function and expr.procedure_type.is_elemental):


This would then become

Suggested change

if self.inline_elementals_only:

if not (expr.procedure_type.is_function and expr.procedure_type.is_elemental):

if self.inline_elementals_only and not expr.procedure_type.is_elemental:

reuterbal · 2024-10-14T16:42:36Z

loki/transformations/inline.py

+            if functions:
+                if expr.routine not in functions:
+                    return
+            if (expr.procedure_type.is_function and expr.procedure_type.is_elemental):


And this would simplify to

Suggested change

if (expr.procedure_type.is_function and expr.procedure_type.is_elemental):

if expr.procedure_type.is_elemental:

MichaelSt98 · 2024-10-14T17:26:35Z

Thanks for the quick review!

… for now, until fixed

github-actions · 2024-10-14T17:41:22Z

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

…ce is array

…ilation

codecov · 2024-10-15T08:49:34Z

Codecov Report

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

Project coverage is 95.58%. Comparing base (b6eb817) to head (06a810e).
Report is 51 commits behind head on main.

Files with missing lines	Patch %	Lines
loki/transformations/inline/functions.py	91.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #401   +/-   ##
=======================================
  Coverage   95.57%   95.58%           
=======================================
  Files         201      201           
  Lines       39810    39874   +64     
=======================================
+ Hits        38050    38112   +62     
- Misses       1760     1762    +2

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`95.56% <98.11%> (+<0.01%)`	⬆️

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.

reuterbal

Thanks for taking care of the suggested changes. Looks great now!

mlange05

Yes, looks good to me. GTG

reuterbal requested changes Oct 14, 2024

View reviewed changes

Inline elemental functions: skip calls with args being array (slices)…

3346735

… for now, until fixed

MichaelSt98 force-pushed the nams-inline-skip-elemental-functions-array-args branch from 4739c2c to 3346735 Compare October 14, 2024 17:38

MichaelSt98 added 2 commits October 15, 2024 10:05

Inline elemental functions: only test whether array (slice) if instan…

c8de338

…ce is array

Inline elemental functions: test - provide module for successful comp…

290f230

…ilation

change/revert (to some extent) logic for inlining of elemental functions

06a810e

MichaelSt98 requested a review from reuterbal October 16, 2024 13:22

reuterbal approved these changes Oct 18, 2024

View reviewed changes

reuterbal added the ready to merge This PR has been approved and is ready to be merged label Oct 18, 2024

mlange05 approved these changes Oct 18, 2024

View reviewed changes

reuterbal merged commit cc30c9e into main Oct 18, 2024
13 checks passed

reuterbal deleted the nams-inline-skip-elemental-functions-array-args branch October 18, 2024 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline elemental functions: skip calls with args being array (slices) #401

Inline elemental functions: skip calls with args being array (slices) #401

MichaelSt98 commented Oct 14, 2024

reuterbal left a comment

reuterbal Oct 14, 2024

reuterbal Oct 14, 2024

reuterbal Oct 14, 2024

MichaelSt98 Oct 14, 2024

reuterbal Oct 14, 2024

reuterbal Oct 14, 2024

reuterbal Oct 14, 2024

MichaelSt98 commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

reuterbal left a comment

mlange05 left a comment

	warning(f'Call to elemental function \'{expr.routine.name}\' with array arguments.'
	warning(f"Call to elemental function '{expr.routine.name}' with array arguments."

-                return
+                return
+            if not expr.procedure_type.is_function:
+                return

		if self.inline_elementals_only:
		if not (expr.procedure_type.is_function and expr.procedure_type.is_elemental):

	if self.inline_elementals_only:
	if not (expr.procedure_type.is_function and expr.procedure_type.is_elemental):
	if self.inline_elementals_only and not expr.procedure_type.is_elemental:

	if (expr.procedure_type.is_function and expr.procedure_type.is_elemental):
	if expr.procedure_type.is_elemental:

Inline elemental functions: skip calls with args being array (slices) #401

Inline elemental functions: skip calls with args being array (slices) #401

Conversation

MichaelSt98 commented Oct 14, 2024

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Oct 14, 2024

Choose a reason for hiding this comment

reuterbal Oct 14, 2024

Choose a reason for hiding this comment

reuterbal Oct 14, 2024

Choose a reason for hiding this comment

MichaelSt98 Oct 14, 2024

Choose a reason for hiding this comment

reuterbal Oct 14, 2024

Choose a reason for hiding this comment

reuterbal Oct 14, 2024

Choose a reason for hiding this comment

reuterbal Oct 14, 2024

Choose a reason for hiding this comment

MichaelSt98 commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

reuterbal left a comment

Choose a reason for hiding this comment

mlange05 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 15, 2024 •

edited

Loading