-
Notifications
You must be signed in to change notification settings - Fork 34
Disable script application to only iom_put #3429
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
base: master
Are you sure you want to change the base?
Changes from 9 commits
97d4099
77b0823
9ada128
52e8df9
228d408
3de798f
8061ceb
d70c54b
483f51d
7fa1410
1a894b5
5cfcdeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,7 @@ | |
| Assignment, Loop, Directive, Node, Reference, CodeBlock, Call, | ||
| Routine, Schedule, IntrinsicCall, StructureReference, IfBlock, | ||
| Operation) | ||
| from psyclone.psyir.symbols import DataSymbol, ArrayType | ||
| from psyclone.psyir.symbols import DataSymbol, ArrayType, ScalarType | ||
| from psyclone.psyir.transformations import ( | ||
| ArrayAssignment2LoopsTrans, HoistLoopBoundExprTrans, HoistLocalArraysTrans, | ||
| HoistTrans, InlineTrans, Maxval2LoopTrans, Sum2LoopTrans, Minval2LoopTrans, | ||
|
|
@@ -205,10 +205,6 @@ def normalise_loops( | |
| :param hoist_argument_expressions: whether to hoist array expressions | ||
| out of the containing Call. | ||
| ''' | ||
| # TODO #3412: This is currently limited to iom_put, we want to expand it | ||
| # throughout the code | ||
| if hoist_argument_expressions: | ||
| iom_put_argument_to_temporary(schedule.walk(Call)) | ||
|
|
||
| if hoist_local_arrays and schedule.name not in CONTAINS_STMT_FUNCTIONS: | ||
| # Apply the HoistLocalArraysTrans when possible, it cannot be applied | ||
|
|
@@ -225,6 +221,9 @@ def normalise_loops( | |
| Reference2ArrayRangeTrans().apply(reference) | ||
| except TransformationError: | ||
| pass | ||
| except Exception as err: | ||
| print(reference, reference.parent.parent.debug_string()) | ||
| raise err | ||
|
|
||
| if loopify_array_intrinsics: | ||
| for intr in schedule.walk(IntrinsicCall): | ||
|
|
@@ -280,6 +279,22 @@ def normalise_loops( | |
| except TransformationError: | ||
| pass | ||
|
|
||
| # TODO #3412: This is currently limited to iom_put, we want to expand it | ||
| # throughout the code | ||
| if hoist_argument_expressions: | ||
| iom_put_argument_to_temporary(schedule.walk(Call)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now no longer limited to iom_put right? So the TODO can be removed and the method name updated?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| normalise_loops( | ||
| schedule, | ||
| hoist_local_arrays=hoist_local_arrays, | ||
| convert_array_notation=convert_array_notation, | ||
| loopify_array_intrinsics=loopify_array_intrinsics, | ||
| convert_range_loops=convert_range_loops, | ||
| scalarise_loops=scalarise_loops, | ||
| increase_array_ranks=False, | ||
| hoist_expressions=hoist_expressions, | ||
| # Make sure we never repeat this step. | ||
| hoist_argument_expressions=False, | ||
| ) | ||
| # TODO #1928: In order to perform better on the GPU, nested loops with two | ||
| # sibling inner loops need to be fused or apply loop fission to the | ||
| # top level. This would allow the collapse clause to be applied. | ||
|
|
@@ -550,13 +565,17 @@ def iom_put_argument_to_temporary(calls: list[Call]): | |
|
|
||
| ''' | ||
| for call in calls: | ||
| if call.symbol.name == "iom_put": | ||
| for arg in call.arguments: | ||
| dtype = arg.datatype | ||
| if (isinstance(dtype, ArrayType) and | ||
| (isinstance(arg, Operation) or | ||
| isinstance(arg, IntrinsicCall))): | ||
| try: | ||
| DataNodeToTempTrans().apply(arg, verbose=True) | ||
| except TransformationError: | ||
| pass | ||
| # if call.symbol.name == "iom_put": | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
| for arg in call.arguments: | ||
| dtype = arg.datatype | ||
| if (isinstance(dtype, ArrayType) and | ||
| (isinstance(arg, Operation) or | ||
| isinstance(arg, IntrinsicCall))): | ||
|
Comment on lines
+567
to
+569
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a comment explaining the condition: "Only extract expressions that can potentially be loopyfied (e.g. operations over arrays)"
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| try: | ||
| if (isinstance(dtype.elemental_type, ScalarType) | ||
| and dtype.elemental_type.intrinsic == | ||
| ScalarType.Intrinsic.CHARACTER): | ||
| continue | ||
|
Comment on lines
+571
to
+574
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this an issue with the transformation? I am wondering if it should be inside the validation instead?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure - the transformation does it successfully but there was some error, I don't remember whether compiler or elsewhere in PSyclone. I can have a further look if you want, I just decided to skip it here for strings (not sure if there would be some other valid use case?) |
||
| DataNodeToTempTrans().apply(arg, verbose=True) | ||
| except TransformationError: | ||
| pass | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,7 +40,7 @@ | |
| from psyclone.configuration import Config | ||
| from psyclone.psyir.frontend.fortran import FortranReader | ||
| from psyclone.psyir.nodes import ( | ||
| Assignment, Reference | ||
| Assignment, Loop, Reference, Routine | ||
| ) | ||
| from psyclone.psyir.symbols import ( | ||
| DataSymbol, INTEGER_TYPE | ||
|
|
@@ -299,6 +299,9 @@ def test_datanodetotemptrans_apply(fortran_reader, fortran_writer, tmp_path): | |
| psyir = fortran_reader.psyir_from_source(code) | ||
| assign = psyir.walk(Assignment)[0] | ||
| dtrans.apply(assign.rhs.operands[1]) | ||
| # Check the tmp symbol is at the routine scope. | ||
| routine = psyir.walk(Routine)[0] | ||
| assert routine.symbol_table.lookup("tmp") is not None | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah |
||
| out = fortran_writer(psyir) | ||
| assert """ integer, allocatable, dimension(:,:) :: tmp | ||
|
|
||
|
|
@@ -309,6 +312,27 @@ def test_datanodetotemptrans_apply(fortran_reader, fortran_writer, tmp_path): | |
| d = c + tmp""" in out | ||
| assert Compile(tmp_path).string_compiles(out) | ||
|
|
||
| # Check the symbol is still created if the assignment is in some other | ||
| # non-routine scope. | ||
| code = """subroutine test() | ||
| integer :: a, b, c | ||
| integer :: i | ||
|
|
||
| do i = 1, 100 | ||
| a = b + c | ||
| end do | ||
| end subroutine test""" | ||
| psyir = fortran_reader.psyir_from_source(code) | ||
| loop = psyir.walk(Loop)[0].detach() | ||
| assign = loop.walk(Assignment)[0] | ||
| dtrans.apply(assign.rhs) | ||
| assert assign.scope.symbol_table.lookup("tmp") is not None | ||
| out = fortran_writer(loop) | ||
| assert """do i = 1, 100, 1 | ||
| tmp = b + c | ||
| a = tmp | ||
| enddo""" in out | ||
|
|
||
| code = """subroutine test() | ||
| real :: a | ||
| integer :: b | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,3 +141,21 @@ def test_apply(fortran_reader, fortran_writer, tmpdir): | |
| result = fortran_writer(psyir) | ||
| assert expected in result | ||
| assert Compile(tmpdir).string_compiles(result) | ||
|
|
||
|
|
||
| def test_nemo5_transerror_case(fortran_reader): | ||
| '''Test that the transformation fails correctly if we have nested sums | ||
| as the reference can't be converted to an arrayreference.''' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you change the name and description of the test to make it generic about the problem tested here, e.g. "the array reference is inside a non-elemental function and therefore won't be converted"?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| code = """subroutine sum_test() | ||
| integer :: n, m | ||
| real , dimension(:, :) :: array | ||
|
|
||
| result = sum(sum(array, dim=2)) * array(n,m) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the validation counts ArrayReference, maybe test that expressions with other arrays like: work as expected.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the first one already doesn't work as expected. This outputs an error message on the fortran_writer stage, and the err.value is also different (if it were reached): and I'm not sure if this is the right place to fix this - I can make some xfailing tests and an issue?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sergisiso I've addressed the other comments but this one is still open if you have any time to let me know how to proceed before another full review.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both errors seem correct:
So I would say no xfail needed?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thats my mistake in the example, I was concerned the resulting code may be changed but I still get the input so thats fine. I'll check the other example you suggested as well.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested the other examples - the error messages are different, but the transformation does fail as expected. |
||
| end subroutine""" | ||
| psyir = fortran_reader.psyir_from_source(code) | ||
| intrinsic_node = psyir.children[0].children[0].rhs.children[0] | ||
| trans = Sum2LoopTrans() | ||
| with pytest.raises(TransformationError) as err: | ||
| trans.apply(intrinsic_node) | ||
| assert ("Can't apply Sum2LoopTrans to SUM(SUM(array, 2)) due " | ||
| "to no ArrayReference nodes present." in str(err.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.
What happens here? Is this a leftover?
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.
Yeah this was debugging.