Disable script application to only iom_put by LonelyCat124 · Pull Request #3429 · stfc/PSyclone

LonelyCat124 · 2026-05-07T09:47:36Z

No description provided.

codecov · 2026-05-07T11:06:23Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (ca8610a) to head (5cfcdeb).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3429   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         391      391           
  Lines       54609    54674   +65     
=======================================
+ Hits        54590    54655   +65     
  Misses         19       19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

LonelyCat124 · 2026-05-07T15:01:31Z

Some failures after this change can be resolved with a change to reference2arrayrange_trans:

            if not ((isinstance(node.parent, IntrinsicCall) and
                 node.parent.intrinsic in REDUCTION_INTRINSICS)
                or node.parent.is_elemental):
                return

instead of
if not node.parent.is_elemental
However, this still fails for stpctl.f90 for somer aeason, as we end up with an UnresolvedType in the gen_decls part of the backend, which is a bit confusing, I'm trying to track that down.

LonelyCat124 · 2026-05-12T09:41:09Z

I'm trying building today and it just seems to work, I will test full integration and see if any others fail.

LonelyCat124 · 2026-05-12T14:28:12Z

From discussion with sergi the previous direction has potential to produce incorrect code.

Instead we should change operation order:

Do the array transformations that don't involve new temporaries
Create new temporaries
Reapply the array transformations.

LonelyCat124 · 2026-05-12T14:36:18Z

Another option is to move the

convert_array_notation
loopify_array_intrinsics
convert_range_loops
hoist_argument_expressions
parts of normalise_loops into a loopifyTrans metatransformation which I wanted to do for while, no individual enabling flags needed.
Then the DataNodeToTempTrans apply can call this loopifyTrans on the extracted expression as it still have them inside the apply.

We don't have metatransformation support yet for this, but I'll make an issue for this as well.

…ay reduction base trans. Testing NYI

LonelyCat124 · 2026-05-15T13:54:10Z

@sergisiso I think this is ready for a first look now.

sergisiso

Thanks @LonelyCat124, its good to see it has passed all integration tests without making it exclusive for iom_put. Just some extra clarifications and testing needed.

sergisiso · 2026-05-19T08:29:04Z

+            except Exception as err:
+                print(reference, reference.parent.parent.debug_string())
+                raise err


What happens here? Is this a leftover?

Yeah this was debugging.

sergisiso · 2026-05-19T08:30:35Z

+    # 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))


This is now no longer limited to iom_put right? So the TODO can be removed and the method name updated?

sergisiso · 2026-05-19T08:31:03Z

-                        DataNodeToTempTrans().apply(arg, verbose=True)
-                    except TransformationError:
-                        pass
+        # if call.symbol.name == "iom_put":


sergisiso · 2026-05-19T08:33:32Z

+                    if (isinstance(dtype.elemental_type, ScalarType)
+                        and dtype.elemental_type.intrinsic ==
+                            ScalarType.Intrinsic.CHARACTER):
+                        continue


Is this an issue with the transformation? I am wondering if it should be inside the validation instead?

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?)

sergisiso · 2026-05-19T08:35:19Z

+            if (isinstance(dtype, ArrayType) and
+                (isinstance(arg, Operation) or
+                    isinstance(arg, IntrinsicCall))):


Maybe a comment explaining the condition: "Only extract expressions that can potentially be loopyfied (e.g. operations over arrays)"

sergisiso · 2026-05-19T08:41:20Z

+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.'''


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"?

sergisiso · 2026-05-19T08:44:44Z

+    integer :: n, m
+    real , dimension(:, :) :: array
+
+    result = sum(sum(array, dim=2)) * array(n,m)


Since the validation counts ArrayReference, maybe test that expressions with other arrays like:
sum(sum(array + array(:,:), dim=2))
or
sum(sum(array, dim=dimensions(2)))

work as expected.

So the first one already doesn't work as expected.

code = """subroutine sum_test() integer :: n, m real, dimension(:, :) :: array result = sum(sum(array + array(:,:), dim=2)) end subroutine""" psyir = fortran_reader.psyir_from_source(code) intrinsic_node = psyir.children[0].children[0].rhs trans = Sum2LoopTrans() with pytest.raises(TransformationError) as err: trans.apply(intrinsic_node) print(fortran_writer(psyir)) print(err.value) assert False

This outputs an error message on the fortran_writer stage, and the err.value is also different (if it were reached):

E psyclone.psyir.backend.visitor.VisitorError: Visitor Error: The following symbols are not explicitly declared or imported from a module and there are no wildcard imports which could be bringing them into scope: 'result'

and

Transformation Error: ArrayAssignment2LoopsTrans could not convert the expression: array(:,:) = SUM(array(:,:) + array(:,:), 2) into a loop because: Transformation Error: ArrayAssignment2LoopsTrans does not support statements containing dependencies that would generate loop-carried dependencies when naively converting them to a loop, but found: array(:,:) = SUM(array(:,:) + array(:,:), 2)

I'm not sure if this is the right place to fix this - I can make some xfailing tests and an issue?

@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.

Both errors seem correct:

result is not declared in the example

And TransformationErrors are fine, what we don't want is to validate and then generate incorrect code.

So I would say no xfail needed?

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.

I tested the other examples - the error messages are different, but the transformation does fail as expected.

sergisiso · 2026-05-19T08:45:39Z

    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


assert "tmp" in routine.symbol_table ?

Disable application to only iom_put

97d4099

LonelyCat124 temporarily deployed to integration May 7, 2026 09:48 — with GitHub Actions Inactive

Don't do it for characters

77b0823

LonelyCat124 temporarily deployed to integration May 7, 2026 10:54 — with GitHub Actions Inactive

Potential fix to datanodetotemptrans, tests NYI]

9ada128

Merge branch 'master' into 3412_nemo_script_improvement

52e8df9

Improvements to Ref2ArrayRange trans

228d408

LonelyCat124 temporarily deployed to integration May 12, 2026 10:26 — with GitHub Actions Inactive

changes to nemo infrastructure to order of operations, and fix to arr…

3de798f

…ay reduction base trans. Testing NYI

LonelyCat124 temporarily deployed to integration May 14, 2026 11:06 — with GitHub Actions Inactive

LonelyCat124 added 2 commits May 14, 2026 15:05

fixed tests and failing cases while maintaining expected results

8061ceb

linting

d70c54b

LonelyCat124 temporarily deployed to integration May 14, 2026 14:18 — with GitHub Actions Inactive

Fix coverage

483f51d

LonelyCat124 requested a review from sergisiso May 15, 2026 13:53

LonelyCat124 marked this pull request as ready for review May 15, 2026 13:53

LonelyCat124 added the ready for review label May 18, 2026

sergisiso requested changes May 19, 2026

View reviewed changes

sergisiso added reviewed with actions and removed ready for review labels May 19, 2026

LonelyCat124 added 3 commits May 19, 2026 13:08

First changes towards the review

7fa1410

linting issue

1a894b5

Added other tests

5cfcdeb

Conversation

LonelyCat124 commented May 7, 2026

Uh oh!

codecov Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LonelyCat124 commented May 7, 2026

Uh oh!

LonelyCat124 commented May 12, 2026

Uh oh!

LonelyCat124 commented May 12, 2026

Uh oh!

LonelyCat124 commented May 12, 2026

Uh oh!

LonelyCat124 commented May 15, 2026

Uh oh!

sergisiso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 7, 2026 •

edited

Loading