(closes #2105) Use maxval2loop and atomics to parallelise NEMO stpctl.f90 #2439

sergisiso · 2023-12-14T16:24:27Z

Updates the nemo scripts with the new maxval2loop and atomic psyclone functionality, this makes it ~10% faster (for my 10its elapsed time test) - although there was a lot of variability today in Glados.

If I look with the profiler:
Before:

After:

The benefit comes from keeping the arrays in the GPU, not so much from stpctl itself as it is dominated by a syscall (IO) and sending this new data to the GPU.

sergisiso · 2023-12-14T16:31:54Z

@arporter @rupertford This is ready to review. (if the reviewer doesn't do it before I will launch the integration tests at night to try to get GLaDos emptier as this PR should give a new baseline for perf comparisons)

codecov · 2023-12-14T16:37:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (106543d) 99.85% compared to head (8bd137c) 99.85%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2439   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files         352      352           
  Lines       47335    47337    +2     
=======================================
+ Hits        47266    47268    +2     
  Misses         69       69

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

arporter

Change looks OK, always good to go faster! I'd prefer a Transformation but that can be a separate Issue :-)
We also need to see about moving code out of examples and into e.g. domain/nemo so that we can add tests. Again, a separate Issue.
I note that the integration tests failed last time but I think that's because of #2440 so will have another go once that's on.

examples/nemo/scripts/omp_gpu_trans.py

examples/nemo/scripts/utils.py

examples/nemo/scripts/omp_gpu_trans.py

sergisiso · 2023-12-20T08:47:54Z

I note that the integration tests failed last time but I think that's because of #2440 so will have another go once that's on.

Unfortunately it was not/only this, it is because a maxval2loop in the normalisation step fails for the esmwf version of nemo.

Invokes found in psy_bdyini_psy:
bdy_def
raised the following exception during execution...
{
      File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/examples/nemo/scripts/omp_cpu_trans.py", line 92, in trans
    normalise_loops(
      File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/examples/nemo/scripts/utils.py", line 179, in normalise_loops
    Maxval2LoopTrans().apply(intr)
      File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.12/site-packages/psyclone/psyir/transformations/intrinsics/array_reduction_base_trans.py", line 224, in apply
    reference2arrayrange.apply(reference)
      File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.12/site-packages/psyclone/psyir/transformations/reference2arrayrange_trans.py", line 177, in apply
    self.validate(node, options=None)
      File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.12/site-packages/psyclone/psyir/transformations/reference2arrayrange_trans.py", line 149, in validate
    if not node.symbol.is_array:
           ^^^^^^^^^^^^^^^^^^^^
      File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.12/site-packages/psyclone/psyir/symbols/symbol.py", line 440, in is_array
    raise ValueError(f"No array information is available for the "
    ValueError: No array information is available for the symbol 'nb_bdy'.

To fix it I modified the generic implementation to return False, with matches the specialised implementation in that if doesn't have enough information (e.g. DeferedType, no partial datatype) it also returns false.

sergisiso · 2023-12-20T13:59:59Z

There is also this issue:

    Maxval2LoopTrans().apply(intr)
      File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.12/site-packages/psyclone/psyir/transformations/intrinsics/array_reduction_base_trans.py", line 349, in apply
    outer_loop.parent.children.insert(outer_loop.position, assignment)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
    AttributeError: 'NoneType' object has no attribute 'children'

sergisiso · 2023-12-20T14:44:36Z

The one that fails is jpbdta = MAXVAL(nblendta(:,:nb_bdy))
Also when doing: x(3) = maxval(a) it initialises with x(3) = -HUGE(x) but should be -HUGE(x(3))?

sergisiso · 2023-12-20T16:54:30Z

Also fails in ecmwf's zcflnow(1) = MAX( zcflnow(1), MAXVAL( ABS( pv_ice(:,:) ) * rdt_ice * r1_e2v(:,:) ) ) with the compile time error The shapes of the array expressions do not conform. [ZCFLNOW] zcflnow(1) = MAX(zcflnow(1), tmp_var)

where zcflnow is:
REAL(wp), DIMENSION(1) :: zcflprv, zcflnow

sergisiso · 2023-12-21T12:13:15Z

@arporter This is ready for another review. It was more complex to fix than I thought. I could have disabled the maxval2loop on the ecmwf version since the CPU will not parallelise them, but I think its good to have them on to expose the potential issues.

rupertford · 2023-12-21T13:49:21Z

The one that fails is jpbdta = MAXVAL(nblendta(:,:nb_bdy)) Also when doing: x(3) = maxval(a) it initialises with x(3) = -HUGE(x) but should be -HUGE(x(3))?

This shouldn't matter. The use of a variable or array should result in the same value of HUGE as they have the same datatype and it is the precision of the datatype that determines the value of HUGE.

sergisiso · 2023-12-21T15:39:41Z

The one that fails is jpbdta = MAXVAL(nblendta(:,:nb_bdy)) Also when doing: x(3) = maxval(a) it initialises with x(3) = -HUGE(x) but should be -HUGE(x(3))?

This shouldn't matter. The use of a variable or array should result in the same value of HUGE as they have the same datatype and it is the precision of the datatype that determines the value of HUGE.

These were 2 different problems:

The first is that nblendta(:,:nb_bdy) was silently not converted to a loop by the arrayrange2loop transformation and the apply continued with the wrong nodes. This is now catch and an error is raised.
The second is it true that for an array HUGE does the right thing, although it is not documented in the standard, but I still think it had to be fixed because the lhs can be a complex expression e.g. "mystryct(3)%myvar" so the huge needs to be initialised by type of the resulting access. And the change was quite simple.

arporter · 2024-01-22T10:14:24Z

nblendta(:,:nb_bdy) was silently not converted to a loop by the arrayrange2loop transformation

Hi @sergisiso, can you remember why this was? It seems like something we should fix (in a separate PR)?

arporter

All looks good now, thanks.
I'll re-run the integration tests, just to make sure.

arporter · 2024-01-26T16:53:20Z

Integration tests were fine.

arporter

All fine now. Will proceed to merge once CI complete.

arporter · 2024-01-26T20:39:24Z

I noticed that CodeCov was reporting newly-missed lines. This was because Symbol.is_array no longer raises an exception so I've removed the try...except around the call to it. I'll re-run the integration tests to be on the safe side.

arporter · 2024-01-26T21:58:11Z

Happily, integration tests were all fine.

#2105 Use maxval2loop and atomics to parallelise NEMO stpctl.f90

1726aad

sergisiso self-assigned this Dec 14, 2023

sergisiso requested review from arporter and rupertford December 14, 2023 16:27

sergisiso added the ready for review label Dec 14, 2023

sergisiso temporarily deployed to integration December 14, 2023 19:05 — with GitHub Actions Inactive

sergisiso and others added 2 commits December 17, 2023 17:41

#2105 Revert ACC maxval reduction with atomics

609d2bd

Merge branch 'master' into 2105_nemo_maxval

23d09e8

arporter added under review and removed ready for review labels Dec 19, 2023

arporter requested changes Dec 19, 2023

View reviewed changes

examples/nemo/scripts/omp_gpu_trans.py Show resolved Hide resolved

examples/nemo/scripts/utils.py Show resolved Hide resolved

examples/nemo/scripts/omp_gpu_trans.py Show resolved Hide resolved

arporter added reviewed with actions and removed under review labels Dec 19, 2023

sergisiso added 2 commits December 19, 2023 17:24

#2105 Fix pylint and add TODO

854babc

Merge remote-tracking branch 'origin/master' into 2105_nemo_maxval

a775f9f

sergisiso temporarily deployed to integration December 19, 2023 22:16 — with GitHub Actions Inactive

#2105 is_array returns false if type can not be determined

8c85171

sergisiso temporarily deployed to integration December 20, 2023 08:58 — with GitHub Actions Inactive

#2439 Fix issues with the array_reduction_base transformation

6b816d0

sergisiso temporarily deployed to integration December 20, 2023 15:45 — with GitHub Actions Inactive

#2105 Fix temporary variables with wrong type

9a0f120

sergisiso temporarily deployed to integration December 21, 2023 10:09 — with GitHub Actions Inactive

sergisiso added ready for review and removed reviewed with actions labels Dec 21, 2023

Merge branch 'master' into 2105_nemo_maxval

c52ecee

arporter added under review and removed ready for review labels Jan 22, 2024

arporter reviewed Jan 22, 2024

View reviewed changes

arporter temporarily deployed to integration January 22, 2024 10:16 — with GitHub Actions Inactive

Merge branch 'master' into 2105_nemo_maxval

598776a

#2439 update changelog and UG

bf6d67c

arporter added ready to be merged and removed under review labels Jan 26, 2024

arporter approved these changes Jan 26, 2024

View reviewed changes

#2439 rm unreachable code in Symbol.is_array_access

8bd137c

arporter temporarily deployed to integration January 26, 2024 20:39 — with GitHub Actions Inactive

arporter merged commit 77b28ff into master Jan 26, 2024
12 checks passed

arporter deleted the 2105_nemo_maxval branch January 26, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(closes #2105) Use maxval2loop and atomics to parallelise NEMO stpctl.f90 #2439

(closes #2105) Use maxval2loop and atomics to parallelise NEMO stpctl.f90 #2439

sergisiso commented Dec 14, 2023

sergisiso commented Dec 14, 2023

codecov bot commented Dec 14, 2023 •

edited

Loading

arporter left a comment

sergisiso commented Dec 20, 2023

sergisiso commented Dec 20, 2023

sergisiso commented Dec 20, 2023

sergisiso commented Dec 20, 2023

sergisiso commented Dec 21, 2023

rupertford commented Dec 21, 2023

sergisiso commented Dec 21, 2023

arporter commented Jan 22, 2024

arporter left a comment

arporter commented Jan 26, 2024

arporter left a comment

arporter commented Jan 26, 2024

arporter commented Jan 26, 2024

(closes #2105) Use maxval2loop and atomics to parallelise NEMO stpctl.f90 #2439

(closes #2105) Use maxval2loop and atomics to parallelise NEMO stpctl.f90 #2439

Conversation

sergisiso commented Dec 14, 2023

sergisiso commented Dec 14, 2023

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

arporter left a comment

Choose a reason for hiding this comment

sergisiso commented Dec 20, 2023

sergisiso commented Dec 20, 2023

sergisiso commented Dec 20, 2023

sergisiso commented Dec 20, 2023

sergisiso commented Dec 21, 2023

rupertford commented Dec 21, 2023

sergisiso commented Dec 21, 2023

arporter commented Jan 22, 2024

arporter left a comment

Choose a reason for hiding this comment

arporter commented Jan 26, 2024

arporter left a comment

Choose a reason for hiding this comment

arporter commented Jan 26, 2024

arporter commented Jan 26, 2024

codecov bot commented Dec 14, 2023 •

edited

Loading