(Closes #1664) Add ACCWaitDirective and async keyword on kernels, enter data, parallel #2664

arporter · 2024-07-18T15:40:27Z

This is a replacement/takeover of #2070 as Sebastien has moved to a new job.

arporter · 2024-07-18T17:50:33Z

Now that I look at this again, there don't seem to be any tests of the new directive and keyword for the Fortran psyir backend (it's all gen_code in LFRic).

arporter · 2024-07-18T17:57:24Z

I see from #2070 (comment) that this PR was started before we had ACCClause (#2157). However, we do now have clauses and therefore this code needs updating (to remove the use of Signature) and tests for the Fortran backend are required.

codecov · 2024-11-22T12:54:29Z

Codecov Report

Attention: Patch coverage is 98.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 99.88%. Comparing base (b051810) to head (8b4e0e9).

Files with missing lines	Patch %	Lines
src/psyclone/psyir/backend/fortran.py	50.00%	2 Missing ⚠️
src/psyclone/psyir/nodes/acc_clauses.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2664      +/-   ##
==========================================
- Coverage   99.88%   99.88%   -0.01%     
==========================================
  Files         359      360       +1     
  Lines       50681    50874     +193     
==========================================
+ Hits        50625    50815     +190     
- Misses         56       59       +3

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

schreiberx · 2024-11-25T22:05:08Z

@arporter

I worked on the code coverage. Let's see what the next push brings.
There was something in the code that I didn't understand:
async_queue was never to None, but then checked against None.
But I found places where async_queue was set to False if the option didn't exist. I changed this throughout the code that async_queue is set per default to None and to an integer otherwise.
Regarding the options dictionary, I set the default argument value to {}. This avoids further checks below ala option is None.

schreiberx · 2024-11-26T11:55:11Z

@arporter

1. I worked on the code coverage. Let's see what the next push brings.

2. There was something in the code that I didn't understand:
   `async_queue` was never to `None`, but then checked against `None`.
   But I found places where `async_queue` was set to `False` if the option didn't exist. I changed this throughout the code that `async_queue` is set per default to `None` and to an integer otherwise.

3. Regarding the options dictionary, I set the default argument value to {}. This avoids further checks below ala `option is None`.

I updated the async part in the previous push.
async_queue can be finally set to these values:

None or False which doesn't make use of it
True which uses it without specifying a concrete queue
An integer or a Reference as the concrete queue to use.

sergisiso

@arporter @schreiberx I don't know who is the owner of this PR but I added my review. My main concern is that it adds PSyIR nodes in a private attribute of a mixin, and this has been a source of issues in the past as these are easy to forget when doing tree modifications. I think we should consider making it a ACCClause so all the tree functionality works as expected.

sergisiso · 2024-12-09T14:24:29Z

src/psyclone/psyir/nodes/acc_directives.py

        self.parallelism = parallelism
+        assert self.parallelism in self.SUPPORTED_PARALLELISM


Remove, this is done in the setter above

sergisiso · 2024-12-09T14:29:39Z

src/psyclone/psyir/nodes/acc_directives.py

+    :param async_queue: Enable async support and attach it to the given queue.
+        Can use False to disable, True to enable on default
+        stream. Int to attach to the given stream ID or use a
+        variable Signature to say at runtime what stream to be


The code does not use a Singature, but a Reference which is better, update the docstring.

We could also consider replacing it with a DataNode to accept operations as they are valid.

Done and moved to DataNode. Updated appropriate test to check this works.

sergisiso · 2024-12-09T14:30:31Z

src/psyclone/psyir/nodes/acc_directives.py

+                self,
+                children: List[Node] = None,
+                parent: Node = None,
+                async_queue: Union[bool, int, Reference, None] = None


Could we do: Union[bool, int, Reference] = False as I assume False and None have the same meaning?

Also everywhere else that has async_queue

I'm not sure they did but I've changed things now so that True means an async clause without an explicit queue while False means no async clause. I've removed the support for None.

sergisiso · 2024-12-09T14:38:52Z

src/psyclone/psyir/nodes/acc_directives.py

+                                Tuple[
+                                    Set[Signature], Set[Signature]
+                                ]


Being picky I slightly prefer would put this tree lines into one. But to make it more readable, but I can also be convinced not too (e.g. is what black choses)

sergisiso · 2024-12-09T14:47:45Z

src/psyclone/psyir/nodes/acc_mixins.py

+''' This module contains the mixins to apply some ACC features on many
+classes.'''


I am tempted to say that this should be in acc_directives.py, as I quite liked that everything regarding OpenACC was encapsulated there, but that file has reached the 1000 (although we will delete gen_code soon - hopefully), so can be convinced otherwise.

I'm fairly relaxed about having another file and as you say, the directives file is at 1.3K now. If we really care we could have an openacc submodule?

sergisiso · 2024-12-09T15:04:44Z

src/psyclone/psyir/transformations/acc_kernels_trans.py

 from psyclone.psyir.symbols import UnsupportedFortranType
 from psyclone.psyir.transformations.region_trans import RegionTrans
 from psyclone.psyir.transformations.transformation_error import (
    TransformationError)

+from typing import Any, Dict, List, Union


pylint wants this before third party libs

sergisiso · 2024-12-09T15:06:16Z

src/psyclone/psyir/transformations/acc_kernels_trans.py

        :param options: a dictionary with options for transformations.
-        :type options: Optional[Dict[str, Any]]
-        :param bool options["default_present"]: whether or not the kernels
+            "default_present": boolean whether or not the kernels


Should this still be :param bool options["default_present"]: as before?

Yes it should. I've also put back the one for "async_queue".

sergisiso · 2024-12-09T15:07:43Z

src/psyclone/psyir/transformations/acc_kernels_trans.py

        :param options: a dictionary with options for transformations.
-        :type options: Optional[Dict[str, Any]]
-        :param bool options["disable_loop_check"]: whether to disable the
+            "disable_loop_check": boolean whether to disable the


Same here and below, I am unsure which format is rendered more clearly in the docs.

I've reverted to our original form.

sergisiso · 2024-12-09T15:10:15Z

src/psyclone/transformations.py

+        The available options are :
+         - async_queue : Permit to force using the given async stream if
+                         not None.


Use common format for option arguments.

sergisiso · 2024-12-09T15:12:00Z

src/psyclone/tests/psyir/transformations/transformations_test.py

@@ -35,6 +35,7 @@
 #         A. B. G. Chalk, STFC Daresbury Lab
 # Modified I. Kavcic, Met Office
 # Modified J. Henrichs, Bureau of Meteorology
+# Modified S. Valat, INRIA / LJK


Maybe delete since at this point there isn't modifications at this specific file (but there is a couple of actually modified files in this PR where the contributors' names could be added).

svalat and others added 7 commits September 11, 2023 16:49

#2070 ACC async_queue via mixins

27d51f9

Merge branch 'master' into feature/acc-wait-directive

701f728

Merge branch 'master' into 1664_acc_wait_dir

833e1f7

#1664 mv new functionality over to new class location

918e07b

#1664 fix some import errors after merge

edf9917

#1664 more fixes following merge

b9de9fe

#1664 fix linting errors

da4f5b5

arporter and others added 3 commits August 8, 2024 09:19

Merge branch 'master' into 1664_acc_wait_dir

4848cd2

Merge branch 'master' into 1664_acc_wait_dir

cda76d4

#1664 update tests

2ef718b

arporter added 2 commits November 22, 2024 14:17

Merge branch 'master' into 1664_acc_wait_dir

4974eda

#1664 tidy and rationalise checking code

b351d84

arporter mentioned this pull request Nov 22, 2024

Add ACCWaitDirective and async keyword on kernels, enter data, parallel (fixed #1664) #2070

Closed

schreiberx self-assigned this Nov 25, 2024

updated for coverage

1be11c3

schreiberx added the ready for review label Nov 25, 2024

SCHREIBER Martin added 4 commits November 26, 2024 00:12

u

48930fd

fixed generation of documentation

ac7783d

updates for CI

81b9038

removed obsolete line => 100% coverage

aefa05d

schreiberx requested a review from sergisiso November 26, 2024 10:07

schreiberx assigned arporter Nov 26, 2024

sergisiso added under review and removed ready for review labels Dec 9, 2024

sergisiso requested changes Dec 9, 2024

View reviewed changes

sergisiso added reviewed with actions and removed under review labels Dec 9, 2024

arporter added in progress and removed reviewed with actions labels Dec 19, 2024

arporter and others added 9 commits December 19, 2024 11:29

Merge branch 'master' into 1664_acc_wait_dir

aab90db

#1664 begin adding new clause [skip ci]

c8a2629

Merge branch 'master' into 1664_acc_wait_dir

1c5a6a7

#1664 more reworking to use Clause [skip ci]

50ddb92

#1664 fix linting [skip ci]

2c677a2

#1664 fix tests

c099396

Merge branch 'master' into 1664_acc_wait_dir

31f0c07

#1664 support DataNode for async queue and tidy docstrings

1fb7c85

#1664 fix lint

8b4e0e9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Closes #1664) Add ACCWaitDirective and async keyword on kernels, enter data, parallel #2664

(Closes #1664) Add ACCWaitDirective and async keyword on kernels, enter data, parallel #2664

arporter commented Jul 18, 2024

arporter commented Jul 18, 2024

arporter commented Jul 18, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

schreiberx commented Nov 25, 2024

schreiberx commented Nov 26, 2024

sergisiso left a comment

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

arporter Jan 10, 2025

sergisiso Dec 9, 2024

sergisiso Dec 9, 2024

		self.parallelism = parallelism
		assert self.parallelism in self.SUPPORTED_PARALLELISM

		''' This module contains the mixins to apply some ACC features on many
		classes.'''

(Closes #1664) Add ACCWaitDirective and async keyword on kernels, enter data, parallel #2664

Are you sure you want to change the base?

(Closes #1664) Add ACCWaitDirective and async keyword on kernels, enter data, parallel #2664

Conversation

arporter commented Jul 18, 2024

arporter commented Jul 18, 2024

arporter commented Jul 18, 2024

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

schreiberx commented Nov 25, 2024

schreiberx commented Nov 26, 2024

sergisiso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2024 •

edited

Loading