handle modulo operator/function for c-like-backends #383

MichaelSt98 · 2024-09-25T14:21:03Z

[TRANSPILATION] handle modulo operator/function for c-like-backends ...

convert

MOD(a, b) to a % b for integer parameters/arguments
MOD(x, y) to fmod(x, y) for floating point parameters/arguments

github-actions · 2024-09-25T14:23:20Z

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

codecov · 2024-09-25T15:04:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.50%. Comparing base (6ecf132) to head (c613469).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #383   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files         185      185           
  Lines       38756    38819   +63     
=======================================
+ Hits        37013    37076   +63     
  Misses       1743     1743

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`95.49% <100.00%> (+<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, I think this needs a rebase and subsequent import fix, the rest is just small nitpicks.

loki/backend/cgen.py

reuterbal · 2024-10-04T16:06:00Z

loki/transformations/transpile/tests/test_transpile.py

+# pylint: disable=too-many-lines
+


We should start splitting the tests into logical units (but that can be a separate PR)

reuterbal · 2024-10-04T16:13:05Z

loki/transformations/transpile/tests/test_transpile.py

+    fcode = f"""
+subroutine transpile_special_functions(in, out)
+  use iso_fortran_env, only: real64
+  implicit none
+  {dtype}{'(kind=real64)' if dtype == 'real' else ''}, intent(in) :: in
+  {dtype}{'(kind=real64)' if dtype == 'real' else ''}, intent(inout) :: out
+
+  if (mod(in{'+ 2._real64' if add_float else ''}, 2{'._real64' if dtype == 'real' else ''}{'+ 0._real64' if add_float else ''}) .eq. 0) then
+    out = 42{'._real64' if dtype == 'real' else ''}
+  else
+    out = 11{'._real64' if dtype == 'real' else ''}
+  endif
+end subroutine transpile_special_functions


A suggestion for a slightly more readable code:

Suggested change

fcode = f"""

subroutine transpile_special_functions(in, out)

use iso_fortran_env, only: real64

implicit none

{dtype}{'(kind=real64)' if dtype == 'real' else ''}, intent(in) :: in

{dtype}{'(kind=real64)' if dtype == 'real' else ''}, intent(inout) :: out

if (mod(in{'+ 2._real64' if add_float else ''}, 2{'._real64' if dtype == 'real' else ''}{'+ 0._real64' if add_float else ''}) .eq. 0) then

out = 42{'._real64' if dtype == 'real' else ''}

else

out = 11{'._real64' if dtype == 'real' else ''}

endif

end subroutine transpile_special_functions

if dtype == 'real':

decl_type = f'{dtype}(kind=real64)'

kind = f'._real64'

else:

decl_type = dtype

kind = ''

fcode = f"""

subroutine transpile_special_functions(in, out)

use iso_fortran_env, only: real64

implicit none

{decl_type}, intent(in) :: in

{decl_type}, intent(inout) :: out

if (mod(in{'+ 2._real64' if add_float else ''}, 2{kind}{'+ 0._real64' if add_float else ''}) .eq. 0) then

out = 42{kind}

else

out = 11{kind}

endif

end subroutine transpile_special_functions

loki/transformations/transpile/tests/test_transpile.py

mlange05

Looks great to me.

mlange05 · 2024-10-07T05:47:27Z

loki/backend/cgen.py

@@ -140,6 +143,25 @@ def map_c_reference(self, expr, enclosing_prec, *args, **kwargs):
    def map_c_dereference(self, expr, enclosing_prec, *args, **kwargs):
        return self.format(' (*%s)', self.rec(expr.expression, PREC_NONE, *args, **kwargs))

+    def map_inline_call(self, expr, enclosing_prec, *args, **kwargs):
+
+        class FindFloatLiterals(ExpressionFinder):


Just a minor nitpick, but I think this is general enough to live in loki.ir.expr_visitor.py

mlange05 · 2024-10-07T05:48:53Z

loki/backend/cgen.py

+            #  instead of the floating-point version ('fmod')
+            #  whenever the mentioned evaluations result in being of kind 'integer' ...
+            #  as an example: 'celing(3.1415)' got an floating point value in it, however it evaluates/returns
+            #  an integer, in that case the wrong modulo function/operation is chosen


[no action] This is indeed a tricky corner case. Solution looks good for now, but might need some more thought when this gets problematic.

Likely the correct approach would be an ExpressionTypeMapper that attempts to determine the return type of an expression (similar to ExpressionDimensionsMapper) - but it will require baking in a lot of knowledge about Fortran intrinsics. Fparser might provide some of that, so it could be feasible.

Definitely way beyond the scope of this PR, though.

Yes! We could also think about implementing a C++ (templated) function mymod() which either calls fmod or uses the modulo operator in dependence of the arguments types.

reuterbal

Many thanks, looks good to me.
@MichaelSt98: Since it'll take a little bit until the backlog of PRs is merged anyway, do you want to move the FindFloatLiterals to the expr_visitor module and add a small test? Let me know, otherwise I'll green-light and merge.

reuterbal · 2024-10-07T07:29:24Z

loki/backend/cgen.py

+            #  instead of the floating-point version ('fmod')
+            #  whenever the mentioned evaluations result in being of kind 'integer' ...
+            #  as an example: 'celing(3.1415)' got an floating point value in it, however it evaluates/returns
+            #  an integer, in that case the wrong modulo function/operation is chosen


Likely the correct approach would be an ExpressionTypeMapper that attempts to determine the return type of an expression (similar to ExpressionDimensionsMapper) - but it will require baking in a lot of knowledge about Fortran intrinsics. Fparser might provide some of that, so it could be feasible.

Definitely way beyond the scope of this PR, though.

MichaelSt98 · 2024-10-07T07:40:04Z

@reuterbal Yes I'll move it to expr_visitor module and add a small test. Do we want to keep the name FindFloatLiterals or name it FindRealLiterals?

reuterbal · 2024-10-07T07:41:12Z

Good point! Real is better

…rs.py and add corresponding tests

reuterbal

That looks great now, many thanks!

reuterbal requested changes Oct 4, 2024

View reviewed changes

MichaelSt98 added 2 commits October 5, 2024 14:00

[TRANSPILATION] handle modulo operator/function for c-like-backends

b25f4cd

improve readability, fix imports, ...

1bc4706

MichaelSt98 force-pushed the nams-cgen-modulo-handling branch from 14e7b37 to 1bc4706 Compare October 5, 2024 12:20

mlange05 approved these changes Oct 7, 2024

View reviewed changes

reuterbal reviewed Oct 7, 2024

View reviewed changes

Move 'FindRealLiterals' (formerly 'FindFloatLiterals') to expr_visito…

2ce21a2

…rs.py and add corresponding tests

reuterbal approved these changes Oct 7, 2024

View reviewed changes

MichaelSt98 and others added 2 commits October 7, 2024 12:29

fix test for OMNI (evaluation of BOZ constants)

a428a3b

Merge branch 'main' into nams-cgen-modulo-handling

c613469

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle modulo operator/function for c-like-backends #383

handle modulo operator/function for c-like-backends #383

MichaelSt98 commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading

reuterbal left a comment

reuterbal Oct 4, 2024

reuterbal Oct 4, 2024

mlange05 left a comment

mlange05 Oct 7, 2024

mlange05 Oct 7, 2024

reuterbal Oct 7, 2024

MichaelSt98 Oct 7, 2024

reuterbal left a comment

reuterbal Oct 7, 2024

MichaelSt98 commented Oct 7, 2024

reuterbal commented Oct 7, 2024

reuterbal left a comment

handle modulo operator/function for c-like-backends #383

Are you sure you want to change the base?

handle modulo operator/function for c-like-backends #383

Conversation

MichaelSt98 commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Oct 4, 2024

Choose a reason for hiding this comment

reuterbal Oct 4, 2024

Choose a reason for hiding this comment

mlange05 left a comment

Choose a reason for hiding this comment

mlange05 Oct 7, 2024

Choose a reason for hiding this comment

mlange05 Oct 7, 2024

Choose a reason for hiding this comment

reuterbal Oct 7, 2024

Choose a reason for hiding this comment

MichaelSt98 Oct 7, 2024

Choose a reason for hiding this comment

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Oct 7, 2024

Choose a reason for hiding this comment

MichaelSt98 commented Oct 7, 2024

reuterbal commented Oct 7, 2024

reuterbal left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2024 •

edited

Loading