Fixed logical evaluation of PRESENT intrinsics on Array variables #341

JoeffreyLegaux · 2024-07-03T12:16:56Z

During inlining_member_procedures, PRESENT intrinsics are evaluated according the the actual presence of the concerned variables in the caller arguments.

However, this evaluation always returns false when evaluating Array variables.

This is due to the fact that we compare the array in the PRESENT call which has only the array's name, with the array declaration (from call.arg_map) which includes the dimensions.

I propose to correct that by checking the existence of the variable in the list of variables names from call.arg_map, instead of checking directly in the variables from call.arg_map.

JoeffreyLegaux · 2024-07-03T12:53:11Z

Example of routine with member call

! test_inline.F90
SUBROUTINE TEST_INLINE(YDXFU, YDCPG_OPTS,  YDMF_PHYS_OUT)

USE CPG_OPTS_TYPE_MOD, ONLY : CPG_OPTS_TYPE
USE PARKIND1         , ONLY : JPIM, JPRB
USE YOMXFU           , ONLY : TXFU
USE MF_PHYS_TYPE_MOD , ONLY : MF_PHYS_OUT_TYPE

IMPLICIT NONE

TYPE(TXFU)              ,INTENT(INOUT)            :: YDXFU
TYPE(CPG_OPTS_TYPE)     ,INTENT(IN)               :: YDCPG_OPTS
TYPE(MF_PHYS_OUT_TYPE)  ,INTENT(IN)               :: YDMF_PHYS_OUT
 

IF (YDXFU%YVISICLD%LACTIVE) THEN
  CALL MEMBER_ROUT (YDXFU%VISICLD, PVMIN=YDMF_PHYS_OUT%VISICLD, PSMAX=0.0_JPRB, PSMIN=-1.0_JPRB)
ENDIF


CONTAINS

SUBROUTINE MEMBER_ROUT (X, PVMIN, PVMAX, PSMIN, PSMAX)

REAL(KIND=JPRB)         ,INTENT(INOUT)            :: X(1:YDCPG_OPTS%KLON)
REAL(KIND=JPRB)         ,INTENT(IN)    ,OPTIONAL  :: PVMIN(1:YDCPG_OPTS%KLON)
REAL(KIND=JPRB)         ,INTENT(IN)    ,OPTIONAL  :: PVMAX(1:YDCPG_OPTS%KLON)
REAL(KIND=JPRB)         ,INTENT(IN)    ,OPTIONAL  :: PSMIN
REAL(KIND=JPRB)         ,INTENT(IN)    ,OPTIONAL  :: PSMAX

IF (PRESENT (PSMIN)) THEN
  X = X + PSMIN
ELSEIF (PRESENT (PSMAX)) THEN
  X = X + PSMAX
ELSEIF (PRESENT (PVMIN)) THEN
  X = X + MINVAL(PVMIN(:))
ELSEIF (PRESENT (PVMAX)) THEN
  X = X + MAXVAL(PVMAX(:))
ENDIF

END SUBROUTINE MEMBER_ROUT 

END SUBROUTINE TEST_INLINE

Example loki script that applies inlining

from loki import Frontend, Sourcefile
from loki.transformations import inline_member_procedures, resolve_associates

source = Sourcefile.from_file('./test_inline.F90', frontend=Frontend.FP)

f = open('./test_inline_transfo.F90', 'w')

for routine in source.subroutines:
    inline_member_procedures(routine)
    f.write(routine.to_fortran())
f.write('\n')
f.close()

Without the proposed change, PRESENT(PVMIN) becomes .false. in the transformed routine.

reuterbal

Thanks for this, nice find and great reproducer!

The change looks good in principal but I suspect we may have an issue if the casing in the present check doesn't match the routine argument. You can check this, e.g., using PRESENT(psmin) and PRESENT(pvmin) in your reproducer. I left a suggestion to work around this as a comment but this is untested!

To make sure we capture this behaviour in the tests, can I please ask that you add your reproducer as a test in https://github.com/ecmwf-ifs/loki/blob/main/loki/transformations/tests/test_inline.py?

Take a look at the other tests in there to draw inspiration how to implement this and test that the outcome is what you expect. Ideally, make sure the test fails without your change in the transformation and then passes after applying the change.

Thanks and let me know if something is unclear.

loki/transformations/inline.py

codecov · 2024-07-03T15:17:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.36%. Comparing base (abf7968) to head (65b58a1).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #341   +/-   ##
=======================================
  Coverage   95.36%   95.36%           
=======================================
  Files         173      173           
  Lines       36902    36914   +12     
=======================================
+ Hits        35191    35204   +13     
+ Misses       1711     1710    -1

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`95.35% <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.

…with optional arguments

reuterbal

Many thanks for adding the test and verifying the case-insensitivity! Some remarks to make the tests pass successfully, otherwise this is good to go!

reuterbal · 2024-07-10T16:18:42Z

loki/transformations/inline.py

@@ -415,7 +415,8 @@ def _map_unbound_dims(var, val):
        check for check in FindInlineCalls().visit(callee.body) if check.function == 'PRESENT'
    )
    present_map = {
-        check: sym.Literal('.true.') if check.arguments[0] in call.arg_map else sym.Literal('.false.')
+        check: sym.Literal('.true.') if check.arguments[0] in [arg.name for arg in call.arg_map] 


Pylint flagged a trailing white space here

reuterbal · 2024-07-10T16:24:20Z

loki/transformations/tests/test_inline.py

@@ -706,6 +706,60 @@ def test_inline_member_routines_with_associate(frontend):
    assert len(assocs) == 2


+@pytest.mark.parametrize('frontend', available_frontends())
+def test_inline_member_routines_with_optionals(frontend):


OMNI currently fails this test, highlighting that the source is not valid fortran at the moment:

INFO Loki:logging.py:123 [Loki::OMNI] Parsing /tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90 ERROR Loki:logging.py:123 Error: Execution of F_Front failed: ERROR Loki:logging.py:123 Full command: F_Front -fleave-comment /tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90 ERROR Loki:logging.py:123 Output of the command: "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 5: "klev" is not a dummy argument. "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 5: "kidia" is not a dummy argument. "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 5: "kfdia" is not a dummy argument. "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 5: "ktdia" is not a dummy argument. "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 7: type txfu not found "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 7: Invalid type spec "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 8: type mf_phys_out_type not found "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 8: Invalid type spec "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 10: invalid left operand of '%' "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 10: invalid left operand of '%' "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 21: type toto not found "/tmp/loki/10428_os7au7hp/omni-a61442a405c4f93e25410eedc4838fd9.f90", line 21: Invalid type spec

In particular, klev, kidia, kfdia, ktdia, and ydmf_phys_out are missing from the subroutine interface, and we should include dummy imports of txfu and mf_phys_out_type. That makes this at least fully correct Fortran code.
OMNI will then still fail the test because it is going to lack the relevant type definitions, so it is fine to exclude it from the frontend parameterisation as xfail.

reuterbal · 2024-07-10T16:25:12Z

loki/transformations/tests/test_inline.py

+    real(kind=8)         ,intent(in)    ,optional  :: pvmax(1:klon)
+    real(kind=8)         ,intent(in)    ,optional  :: psmin
+    real(kind=8)         ,intent(in)    ,optional  :: psmax
+    type(toto), intent(in), optional :: dfmin


dfmin should also be added to the interface of member_rout, and a corresponding import of toto should be added.

mlange05

@JoeffreyLegaux Many thanks for the fixes. This looks good now and should be ready to go.

Fixed logical evaluation of PRESENT intrinsics on Array variables

5b4fc70

reuterbal requested changes Jul 3, 2024

View reviewed changes

loki/transformations/inline.py Outdated Show resolved Hide resolved

JoeffreyLegaux added 2 commits July 5, 2024 15:00

Added test for evaluation on PRESENT intrinsic when inlining routine …

fd410b6

…with optional arguments

Split statement across multiple line to avoid excessive line length

bf30dfa

reuterbal requested changes Jul 10, 2024

View reviewed changes

JoeffreyLegaux and others added 6 commits August 9, 2024 10:59

removed trailing whitespace

c4711cb

removed useless variable declaration from test

a62719f

removed useless variables declarations from test

524f8fc

added missing argument and imports

94850ca

exclude OMNI frontend from test

952a779

Merge branch 'main' into bugfix-inlining-present-array

65b58a1

mlange05 approved these changes Aug 12, 2024

View reviewed changes

mlange05 added the ready for merge This PR has been approved and is ready to be merged label Aug 12, 2024

reuterbal approved these changes Aug 13, 2024

View reviewed changes

mlange05 merged commit a0207ce into ecmwf-ifs:main Aug 13, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed logical evaluation of PRESENT intrinsics on Array variables #341

Fixed logical evaluation of PRESENT intrinsics on Array variables #341

JoeffreyLegaux commented Jul 3, 2024 •

edited

Loading

JoeffreyLegaux commented Jul 3, 2024

reuterbal left a comment

codecov bot commented Jul 3, 2024 •

edited

Loading

reuterbal left a comment

reuterbal Jul 10, 2024

reuterbal Jul 10, 2024

reuterbal Jul 10, 2024

mlange05 left a comment

Fixed logical evaluation of PRESENT intrinsics on Array variables #341

Fixed logical evaluation of PRESENT intrinsics on Array variables #341

Conversation

JoeffreyLegaux commented Jul 3, 2024 • edited Loading

JoeffreyLegaux commented Jul 3, 2024

reuterbal left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Jul 10, 2024

Choose a reason for hiding this comment

reuterbal Jul 10, 2024

Choose a reason for hiding this comment

reuterbal Jul 10, 2024

Choose a reason for hiding this comment

mlange05 left a comment

Choose a reason for hiding this comment

JoeffreyLegaux commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading