Changes to OpenMP scripts to extract arguments from iom_put by LonelyCat124 · Pull Request #3373 · stfc/PSyclone

LonelyCat124 · 2026-03-13T13:21:53Z

No description provided.

codecov · 2026-03-13T13:34:56Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (ddc5397) to head (5ad2f3e).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3373   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         389      389           
  Lines       54541    54591   +50     
=======================================
+ Hits        54522    54572   +50     
  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-03-15T14:54:29Z

Transformation failing on nemo5, and maybe not for structure of array:

psyclone --enable-cache -l output -s /archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/examples/nemo/scripts/omp_cpu_trans.py -I /archive/psyclone-tests/latest-run/UKMO-NEMOv5/tests/BENCH_OMP_THREADING_GCC/BLD/tmp -o icewri.psycloned.f90 /archive/psyclone-tests/latest-run/UKMO-NEMOv5/tests/BENCH_OMP_THREADING_GCC/BLD/ppsrc/nemo/icewri.f90
Adding OpenMP threading to subroutine: ice_wri
Traceback (most recent call last):
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/bin/psyclonefc", line 45, in <module>
    compiler_wrapper(sys.argv[1:])
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.14/site-packages/psyclone/psyclonefc_cli.py", line 140, in compiler_wrapper
    main(psyclone_args)
    ~~~~^^^^^^^^^^^^^^^
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.14/site-packages/psyclone/generator.py", line 743, in main
    code_transformation_mode(
    ~~~~~~~~~~~~~~~~~~~~~~~~^
        input_file=args.filename,
        ^^^^^^^^^^^^^^^^^^^^^^^^^
    ...<6 lines>...
        line_length=args.limit,
        ^^^^^^^^^^^^^^^^^^^^^^^
        free_form=free_form)
        ^^^^^^^^^^^^^^^^^^^^
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.14/site-packages/psyclone/generator.py", line 964, in code_transformation_mode
    trans_recipe(psyir)
    ~~~~~~~~~~~~^^^^^^^
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/examples/nemo/scripts/omp_cpu_trans.py", line 113, in trans
    iom_put_argument_to_temporary(subroutine.walk(Call))
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/examples/nemo/scripts/utils.py", line 541, in iom_put_argument_to_temporary
    DataNodeToTempTrans().apply(arg)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.14/site-packages/psyclone/psyir/transformations/datanode_to_temp_trans.py", line 274, in apply
    node.scope.symbol_table.add(sym_copy)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.14/site-packages/psyclone/psyir/symbols/symbol_table.py", line 600, in add
    raise KeyError(f"Symbol table already contains a symbol with "
                   f"name '{new_symbol.name}'.")
KeyError: "Symbol table already contains a symbol with name 'Nie0'."

…a lookup

…put_to_temp_

LonelyCat124 · 2026-03-17T12:14:59Z

ITs all pass now, the remaining question is whether there's any performance degredation - I don't think there should be but its possible this is being done "too widely". One for either @sergisiso or @arporter to review.

sergisiso

@LonelyCat124 The changes look good, but I would like that the transformation provide more feedback in order to understand why it hasn't given a performance improvement. Also, see if it can be more generic.

sergisiso · 2026-03-17T12:31:36Z

+        if call.symbol.name == "iom_put":
+            arg = call.arguments[1]
+            dtype = arg.datatype
+            if isinstance(dtype, ArrayType) and isinstance(arg, Operation):
+                try:
+                    DataNodeToTempTrans().apply(arg)
+                except TransformationError:
+                    pass


The integration tests don't show any performance advantage which is not what we expected (we are changing from multiple gpu->cpu reads to one, and to maybe preventing the data touched from the gpu to be brought back):

I can check with a grep how many more loops are offloaded but for the places that it was not applied, could you add as preceding comment the reason why not (if not all transformation errors provide useful information, a verbose option like other transformation have can help)

There is nothing specific of iom_put, other than we know it is a common pattern. We want to avoid touching things from the CPU as much as possible, could this be applied to all subroutine calls (not functions)?

I've added a preceding comment now. I'll try generalising it as well.

This currently causes stuff to fail, I'll try to see if I can get my VPN to start working again and see if I can try manually building NEMO5 to find the cause.

This has shown a few issues with the DataNodeToTempTrans (partly because some things are Statements that I didn't think, e.g. an IfBlock's condition).

Fixed the bugs now

sergisiso · 2026-03-17T12:56:54Z

The DataNode2Temp has a:

calls = node.walk(Call)
for call in calls:
    if not call.is_pure:
       raise

This may be a problem as walk returns self, and iom_put is not pure. I suppose this was meant for the calls in the arguments and we just forgot to skip self?

LonelyCat124 · 2026-03-17T13:25:12Z

The DataNode2Temp has a:
calls = node.walk(Call)
for call in calls:
    if not call.is_pure:
       raise
This may be a problem as walk returns self, and iom_put is not pure. I suppose this was meant for the calls in the arguments and we just forgot to skip self?

@sergisiso I don't think so. We're calling the transformation on the argument, not the call so the iom_put is the parent.

LonelyCat124 · 2026-04-21T14:10:40Z

@sergisiso This is ready for another look now at last.

sergisiso

@LonelyCat124 The implementation is getting closer, and the generated NEMO diff seems to make sense, but it is a bit puzzling that the performance is not getting better. I will have a look with the profiler but I won't stop the PR while I do it.

sergisiso · 2026-04-23T08:55:04Z

+                    node.argument_by_name("l").
+                    datatype.intrinsic,


Keep this in the same line.

Wasn't done? There is also the same unecessary line breaks in lines 3775, 4458, 4730 and 4755

Ah my bad, I reverted some commits to the file I think and so this was also reverted.

Fixed those now.

sergisiso · 2026-04-23T10:38:54Z

+                    try:
+                        DataNodeToTempTrans().apply(arg)
+                    except TransformationError as err:
+                        call.append_preceding_comment(


Also, consider doing the same but inside the transformation with a verbose option.

Done, should be covered by tests too.

Thanks, now you can delete this in favour of the option.

sergisiso · 2026-04-23T10:57:15Z

        if isinstance(self.intrinsic.return_type, Callable):
            try:
                return self.intrinsic.return_type(self)
+            except TypeError as err:


I know we discussed this before but can you remind me why we don't simply use:

try: return self.intrinsic.return_type(self) except (TypeError, AttributeError): return UnresolvedType()

I know for debugging the more explicit error could help, but from a user point of view it doesn't matter that much it would be better to get the UnresolvedType to communicate that we can't do it but let it continue.

I think I'd prefer to be more precise as from a user point of view I think it should never reach the InternalError, so its mostly being defensiveness from a development standpoint to avoid reaching things that are unexpected, but I'm happy to change it if you would prefer.

Ok (it still feels not ideal that we compare against the error message, hopefully we can improve on this in the furture)

…piler can do better

LonelyCat124 · 2026-04-24T13:15:12Z

@sergisiso This is ready for another look, I am going to set ITs going again since output code changes will likely happen.

sergisiso

@LonelyCat124 I am happy with the current implementation and bringing the remaining rough edges to separate issues/TODOs. There is just final clean up to do before merging it.

sergisiso · 2026-04-27T08:45:24Z

+        # Extract any array operations from iom_put calls to temporary
+        # expressions that can be parallelised.


Delete this now?

sergisiso · 2026-04-27T08:45:57Z

        MaximalProfilingOutsideDirectivesTrans().apply(children)
+
+
+def iom_put_argument_to_temporary(calls: list[Call]):


"param:" missing

Added. I also have slightly changed the behaviour so I will rerun integration.

sergisiso · 2026-04-27T08:46:42Z

+                    try:
+                        DataNodeToTempTrans().apply(arg)
+                    except TransformationError as err:
+                        call.append_preceding_comment(


Thanks, now you can delete this in favour of the option.

sergisiso · 2026-04-27T08:49:23Z

-                                  allocatable_datatype.shape])
+            # If any of the bound information aren't static then we need
+            # to create an allocatable array.
+            is_static = True


maybe r/is_static/has_static_bounds/ to not confuse it with symbols with static interface

Yeah thats probably better.

sergisiso · 2026-04-27T08:52:10Z

+                    node.argument_by_name("l").
+                    datatype.intrinsic,


Wasn't done? There is also the same unecessary line breaks in lines 3775, 4458, 4730 and 4755

sergisiso · 2026-04-27T08:53:59Z

        if isinstance(self.intrinsic.return_type, Callable):
            try:
                return self.intrinsic.return_type(self)
+            except TypeError as err:


Ok (it still feels not ideal that we compare against the error message, hopefully we can improve on this in the furture)

sergisiso · 2026-04-27T08:55:50Z

+                # Is this reachable? Tested via monkeypatch as there may be
+                # some edge case I can't think of.


I would prefer a comment like "This should never happen, so propagate as an InternalError" instead.

Yeah, thats neater, the previous comment probably should have been labelled as FIXME, updated now.

LonelyCat124 · 2026-04-27T13:37:46Z

@sergisiso I've set integration running again, assuming no issues arise this is ready for another look. If the tests fail on treesitter related issues then I think we should wait until #3408 is merged.

Changes to OpenMP scripts to extract arguments from iom_put

52b962a

LonelyCat124 temporarily deployed to integration March 13, 2026 13:22 — with GitHub Actions Inactive

transformation can't always work so catch exception

dae4e86

LonelyCat124 temporarily deployed to integration March 13, 2026 14:22 — with GitHub Actions Inactive

Merge branch 'master' into iom_put_to_temp_

34aeaac

LonelyCat124 temporarily deployed to integration March 13, 2026 14:47 — with GitHub Actions Inactive

LonelyCat124 added 3 commits March 16, 2026 13:16

fix to datanode to temp tarns to handle case sensitivity correctly vi…

a95ece0

…a lookup

Merge branch 'iom_put_to_temp_' of github.com:stfc/PSyclone into iom_…

d79fae1

…put_to_temp_

Merge branch 'master' into iom_put_to_temp_

46d6dfa

LonelyCat124 temporarily deployed to integration March 16, 2026 13:18 — with GitHub Actions Inactive

Merge branch 'master' into iom_put_to_temp_

6339e55

LonelyCat124 temporarily deployed to integration March 17, 2026 10:40 — with GitHub Actions Inactive

LonelyCat124 marked this pull request as ready for review March 17, 2026 12:14

LonelyCat124 requested review from arporter and sergisiso and removed request for arporter March 17, 2026 12:15

LonelyCat124 added the ready for review label Mar 17, 2026

LonelyCat124 requested a review from arporter March 17, 2026 12:15

sergisiso added under review and removed ready for review labels Mar 17, 2026

sergisiso requested changes Mar 17, 2026

View reviewed changes

sergisiso added reviewed with actions and removed under review labels Mar 17, 2026

sergisiso mentioned this pull request Mar 17, 2026

(closes #3022) Use datatype to infer the temporary variable when converting reduction intrinsics to code #3359

Merged

Try extending the iom_put transformation

6ce8a38

LonelyCat124 added ready for review and removed reviewed with actions labels Apr 21, 2026

Merge branch 'master' into iom_put_to_temp_

19d77ef

sergisiso added under review and removed ready for review labels Apr 22, 2026

sergisiso requested changes Apr 23, 2026

View reviewed changes

sergisiso added reviewed with actions and removed under review labels Apr 23, 2026

Some changes towards review

fa35a58

LonelyCat124 mentioned this pull request Apr 24, 2026

StructureType.datatype can return an ArrayType with an ArrayType intrinsic. #3415

Open

LonelyCat124 added 3 commits April 24, 2026 13:34

Revert changes due to issue with structure reference's datatype

78b4345

linting

93c0b20

DataNodeToTempTrans will sometimes not make an allocatable if the com…

51c250a

…piler can do better

LonelyCat124 added ready for review and removed reviewed with actions labels Apr 24, 2026

LonelyCat124 requested a review from sergisiso April 24, 2026 13:14

Merge branch 'master' into iom_put_to_temp_

5ad2f3e

LonelyCat124 temporarily deployed to integration April 24, 2026 13:15 — with GitHub Actions Inactive

sergisiso removed the ready for review label Apr 27, 2026

sergisiso requested changes Apr 27, 2026

View reviewed changes

sergisiso added the reviewed with actions label Apr 27, 2026

LonelyCat124 added 2 commits April 27, 2026 14:31

Changes for review

18dae4e

changes for review

b89289f

LonelyCat124 deployed to integration April 27, 2026 13:36 — with GitHub Actions Active

LonelyCat124 added ready for review and removed reviewed with actions labels Apr 27, 2026

LonelyCat124 requested a review from sergisiso April 27, 2026 13:37

		# Extract any array operations from iom_put calls to temporary
		# expressions that can be parallelised.

		MaximalProfilingOutsideDirectivesTrans().apply(children)


		def iom_put_argument_to_temporary(calls: list[Call]):

		# Is this reachable? Tested via monkeypatch as there may be
		# some edge case I can't think of.

Conversation

LonelyCat124 commented Mar 13, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

LonelyCat124 commented Mar 15, 2026

Uh oh!

LonelyCat124 commented Mar 17, 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!

sergisiso commented Mar 17, 2026

Uh oh!

LonelyCat124 commented Mar 17, 2026

Uh oh!

LonelyCat124 commented Apr 21, 2026

Uh oh!

sergisiso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

LonelyCat124 commented Apr 24, 2026

Uh oh!

sergisiso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

codecov Bot commented Mar 13, 2026 •

edited

Loading

sergisiso left a comment •

edited

Loading