bugfix : symbols from the inlined member's dummies were hoisted in the calling routine

[JoeffreyLegaux]

We want to eliminate the symbols from dummies variables when hoisting the local variables from an inlined member routine to its caller.
The check eliminated symbols from the routine._dummies list, but we rather want the list of memeber._dummies

[codecov]

Codecov Report

Merging #162 (0f1f9d7) into main (19fd1c4) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #162   +/-   ##
=======================================
  Coverage   92.10%   92.10%           
=======================================
  Files          90       90           
  Lines       16628    16628           
=======================================
  Hits        15315    15315           
  Misses       1313     1313           

Flag	Coverage Δ
lint_rules	96.22% <ø> (ø)
loki	92.08% <100.00%> (ø)
transformations	91.25% <ø> (ø)

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

Files	Coverage Δ
loki/transform/transform_inline.py	87.21% <100.00%> (ø)

[mlange05]

Hi @JoeffreyLegaux I'm not sure I agree with your assessment that we want to keep arguments of the child routine in decls and thus hoist them to the parent. Could you please provide an example or test for the behaviour that breaks with the current implementation and is fixed by this PR? (Apologies if I'm misunderstanding the intention of you fix here; I find these things easier to comprehend in code.)

For context, the original idea of the implementation is that all arguments to the child are replaced symbolically with the remapped version of the value passed to the call in a particular call context. That means, only purely locally defined variables need to be hoisted up to the parent context, as these are not replaced by the remapping. The decls tuple in this line is simply filtering the local declarations from the argument declarations, so that we may only hoist those local temporaries. Do you have a case where argument declarations need to be hoisted too?

[JoeffreyLegaux]

Hello @mlange05,

I guess my comment was not perfectly clear as I thought the change in the code was self-explanatory.
I totally agree with your comment and certainly would not want to hoist the member's arguments declarations.

But that is not what is happening currently, you can check it out with a simple example.

If you look at the code, here line 245 gets the list of variables in the member spec.
Then line 246 excludes from the previous list the symbols that are in the calling routine's dummies.
What we want is to exclude the list of the member's dummies (which correspond to non-local variables in the member) instead, hence my proposal.

[JoeffreyLegaux]

Here is a very simple test case that highlights the issue :

SUBROUTINE TEST_INLINE()
IMPLICIT NONE

REAL(KIND=JPRB) :: ROUT_ARRAY(KLON), ROUT_ARG(JLEV, KLON) ! instantaneous flux of total precipitations

CALL MEMB_ROUT (ROUT_ARG)

CONTAINS

SUBROUTINE MEMB_ROUT (MEMB_ARRAY)

REAL(KIND=JPRB) ,INTENT(IN) :: MEMB_ARRAY(:,:)
INTEGER :: MEMB_LOCAL

  MEMB_LOCAL = JLEV
  ROUT_ARRAY(KLON) = MEMB_ARRAY(JLEV, KLON)

END SUBROUTINE MEMB_ROUT
END SUBROUTINE TEST_INLINE

After inlining, both the declarations of MEMB_ARRAY and MEMB_LOCAL get hoisted with the current implementation, while we obviously only want the latter.

[mlange05]

@JoeffreyLegaux Ahhh, I see now. Yes, you are right, we are accidentally hoisting the old declaration, because the filtering is wrong, and yes your change fixes that! We still need to add a test to catch this in the future, but one of the existing ones should be ok for that. I'll try and add that to your branch.

[mlange05]

Ok, this is looking good now. Many thanks @JoeffreyLegaux for finding this and filing a bug, and thanks for the explanation/code example.