-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
Transformation utility to fix sequence association #173
Merged
reuterbal
merged 33 commits into
ecmwf-ifs:main
from
rolfhm:sbrm-fix-scalar-syntax-for-array-arguments
Nov 13, 2023
Merged
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
e722250
identified arrays passed as scalars
rolfhm 294b89e
scalars sort of fixed
rolfhm d576e99
commit call
rolfhm 5542be9
Merge branch 'main' of github.com:ecmwf-ifs/loki into sbrm-fix-scalar…
rolfhm 40b90d6
constructed new arguments
rolfhm f46453e
Might work now
rolfhm 3e7a28c
Merge branch 'main' of github.com:ecmwf-ifs/loki into sbrm-fix-scalar…
rolfhm 2afa472
Check that we have the called routine definition
rolfhm e68135b
some documentation
rolfhm 2653b84
simplify TypedSymbol handling
rolfhm 7560abb
some cleanup
rolfhm afb17af
Simplify (?) and add docstrings
rolfhm b6b66cc
fix style
rolfhm 674a6f5
more style
rolfhm c2ec915
even more style
rolfhm 9c2f441
Ensure Loki versions of Sum and Product
rolfhm b3b78ca
fix another negative bug
rolfhm 7fac0d2
add tests
rolfhm b546733
going out in style
rolfhm 32f9cdf
Merge branch 'main' of github.com:ecmwf-ifs/loki into sbrm-fix-scalar…
rolfhm 94c89c6
Add option for turning scalar fix on and off
rolfhm 85e93bf
moved option setting a bit
rolfhm bf0590e
fix linter complaints
rolfhm e0c3f26
Merge branch 'main' of github.com:ecmwf-ifs/loki into sbrm-fix-scalar…
rolfhm 64a959d
scalar_syntax -> sequence_association
rolfhm 0259596
Merge branch 'main' of github.com:ecmwf-ifs/loki into sbrm-fix-scalar…
rolfhm 8f069d5
fix -> resolve
rolfhm 95e9cbf
inline and sequence association tests in single column transformation
rolfhm 286fb8d
cleanup
rolfhm 94a0f6a
greatly simplify by using callers dimensions
rolfhm 80435d7
cleanup
rolfhm 60c3a5b
more cleanup
rolfhm 4d584fa
Merge branch 'main' of github.com:ecmwf-ifs/loki into sbrm-fix-scalar…
rolfhm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
# (C) Copyright 2018- ECMWF. | ||
# This software is licensed under the terms of the Apache Licence Version 2.0 | ||
# which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. | ||
# In applying this licence, ECMWF does not waive the privileges and immunities | ||
# granted to it by virtue of its status as an intergovernmental organisation | ||
# nor does it submit to any jurisdiction. | ||
|
||
from loki.expression import Array, RangeIndex | ||
from loki.ir import CallStatement | ||
from loki.visitors import FindNodes, Transformer | ||
from loki.tools import as_tuple | ||
from loki.types import BasicType | ||
|
||
|
||
__all__ = [ | ||
'transform_sequence_association' | ||
] | ||
|
||
def check_if_scalar_syntax(arg, dummy): | ||
""" | ||
Check if an array argument, arg, | ||
is passed to an array dummy argument, dummy, | ||
using scalar syntax. i.e. arg(1,1) -> d(m,n) | ||
|
||
Parameters | ||
---------- | ||
arg: variable | ||
dummy: variable | ||
""" | ||
if isinstance(arg, Array) and isinstance(dummy, Array): | ||
if arg.dimensions: | ||
if not any(isinstance(d, RangeIndex) for d in arg.dimensions): | ||
return True | ||
return False | ||
|
||
|
||
def transform_sequence_association(routine): | ||
""" | ||
Housekeeping routine to replace scalar syntax when passing arrays as arguments | ||
For example, a call like | ||
|
||
real :: a(m,n) | ||
|
||
call myroutine(a(i,j)) | ||
|
||
where myroutine looks like | ||
|
||
subroutine myroutine(a) | ||
real :: a(5) | ||
end subroutine myroutine | ||
|
||
should be changed to | ||
|
||
call myroutine(a(i:m,j) | ||
|
||
Parameters | ||
---------- | ||
routine : :any:`Subroutine` | ||
The subroutine where calls will be changed | ||
""" | ||
|
||
#List calls in routine, but make sure we have the called routine definition | ||
calls = (c for c in FindNodes(CallStatement).visit(routine.body) if not c.procedure_type is BasicType.DEFERRED) | ||
call_map = {} | ||
|
||
for call in calls: | ||
|
||
new_args = [] | ||
|
||
found_scalar = False | ||
for dummy, arg in call.arg_map.items(): | ||
if check_if_scalar_syntax(arg, dummy): | ||
found_scalar = True | ||
|
||
n_dims = len(dummy.shape) | ||
new_dims = [] | ||
for s, lower in zip(arg.shape[:n_dims], arg.dimensions[:n_dims]): | ||
|
||
if isinstance(s, RangeIndex): | ||
new_dims += [RangeIndex((lower, s.stop))] | ||
else: | ||
new_dims += [RangeIndex((lower, s))] | ||
|
||
if len(arg.dimensions) > n_dims: | ||
new_dims += arg.dimensions[len(dummy.shape):] | ||
new_args += [arg.clone(dimensions=as_tuple(new_dims)),] | ||
else: | ||
new_args += [arg,] | ||
|
||
if found_scalar: | ||
call_map[call] = call.clone(arguments = as_tuple(new_args)) | ||
|
||
if call_map: | ||
routine.body = Transformer(call_map).visit(routine.body) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
# (C) Copyright 2018- ECMWF. | ||
# This software is licensed under the terms of the Apache Licence Version 2.0 | ||
# which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. | ||
# In applying this licence, ECMWF does not waive the privileges and immunities | ||
# granted to it by virtue of its status as an intergovernmental organisation | ||
# nor does it submit to any jurisdiction. | ||
|
||
import pytest | ||
|
||
from conftest import available_frontends | ||
|
||
from loki.backend import fgen | ||
from loki.transform import transform_sequence_association | ||
from loki.module import Module | ||
from loki.ir import CallStatement | ||
from loki.visitors import FindNodes | ||
|
||
@pytest.mark.parametrize('frontend', available_frontends()) | ||
def test_transform_scalar_notation(frontend): | ||
fcode = """ | ||
module mod_a | ||
implicit none | ||
|
||
type type_b | ||
integer :: c | ||
integer :: d | ||
end type type_b | ||
|
||
type type_a | ||
type(type_b) :: b | ||
end type type_a | ||
|
||
contains | ||
|
||
subroutine main() | ||
|
||
type(type_a) :: a | ||
integer :: k, m, n | ||
|
||
real :: array(10,10) | ||
|
||
call sub_x(array(1, 1), 1) | ||
call sub_x(array(2, 2), 2) | ||
call sub_x(array(m, 1), k) | ||
call sub_x(array(m-1, 1), k-1) | ||
call sub_x(array(a%b%c, 1), a%b%d) | ||
|
||
contains | ||
|
||
subroutine sub_x(array, k) | ||
|
||
integer, intent(in) :: k | ||
real, intent(in) :: array(k:n) | ||
|
||
end subroutine sub_x | ||
|
||
end subroutine main | ||
|
||
end module mod_a | ||
""".strip() | ||
|
||
module = Module.from_source(fcode, frontend=frontend) | ||
routine = module['main'] | ||
|
||
transform_sequence_association(routine) | ||
|
||
calls = FindNodes(CallStatement).visit(routine.body) | ||
|
||
assert fgen(calls[0]).lower() == 'call sub_x(array(1:10, 1), 1)' | ||
assert fgen(calls[1]).lower() == 'call sub_x(array(2:10, 2), 2)' | ||
assert fgen(calls[2]).lower() == 'call sub_x(array(m:10, 1), k)' | ||
assert fgen(calls[3]).lower() == 'call sub_x(array(m - 1:10, 1), k - 1)' | ||
assert fgen(calls[4]).lower() == 'call sub_x(array(a%b%c:10, 1), a%b%d)' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add here something like
to cover the other cases in
product_value
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking into it, but discovered that the code crashes if the dimension inside the routine is given by a Sum or a Product, for example
dimension(k+1)
anddimension(-k)
, respectively.The solution is to modify the
process_symbol
routine to recursively process such cases, but I'm wondering if I've been really overthinking the problem. Since sequence association is always contiguous, I think we could just plug in the relevant array dimensions from the caller routine. At least the two calls in this example case produce the same output:Would the inliner have any problem with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, simplicity is always better and this looks really neat now! Pylint has flagged a few redundant symbols and imports now.
I don't think the inliner should have a problem with this, at least there is a test that includes ranges:
loki/tests/test_transform_inline.py
Line 374 in a2f2428