(closes #1010, #1910, #719, #798, #1648) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) #2834

sergisiso · 2024-12-29T17:53:54Z

No description provided.

…ns to PSyIR

…10_remaining_lfric_lowering

…rsor on invokes declarations and initialisations

… invoke comments

…nto 1010_remaining_lfric_lowering

…c_lowering

…ith psyir backend

…r backend

…ster

sergisiso · 2025-01-30T14:00:52Z

Regarding cursor:

I remove the shared "declarations" in favour of "stub_decalrations" and "invoke_declarations" as I think they are sufficiently different to be explicit about which one we are calling.
I removed "cursor" from all declaration methods.
I moved the check that it comes from invoke or kernel to the base implementations and call super() in all these methods, as this was not consistently checked.

However:

I have not merged it with "initialisations" because I am not sure we were sure about it, but more importantly a quick test merging the loops below make many tests fails. Some with wrong output asserts (expected because we change the order that symbols are declared), some with symbol_table lookup errors. I expect we will change the order again when we move this logic to happen before the transformations, so I don't thing it is worth to change all test declaration order now to do it again soon.

PSyclone/src/psyclone/domain/lfric/lfric_invoke.py

Lines 285 to 303 in 1a67f95

    
           for entities in [self.scalar_args, self.fields, self.lma_ops, 
        
                            self.stencil, self.meshes, 
        
                            self.function_spaces, self.dofmaps, self.cma_ops, 
        
                            self.boundary_conditions, self.evaluators, 
        
                            self.halo_depths, 
        
                            self.proxies, self.cell_iterators, 
        
                            self.reference_element_properties, 
        
                            self.mesh_properties, self.loop_bounds, 
        
                            self.run_time_checks]: 
        
               cursor = entities.invoke_declarations(cursor) 
        
           for entities in [self.proxies, self.run_time_checks, 
        
                            self.cell_iterators, self.meshes, 
        
                            self.stencil, self.dofmaps, 
        
                            self.cma_ops, self.boundary_conditions, 
        
                            self.function_spaces, self.evaluators, 
        
                            self.reference_element_properties, 
        
                            self.mesh_properties, self.loop_bounds]: 
        
               cursor = entities.initialise(cursor)

sergisiso · 2025-02-03T12:06:59Z

@arporter This is ready for another review. The PR has increased a bit from last time but mostly with the same syntax changes applied to GOcean tests and lots of deletions - which is satisfying. As a result, a few more issues can be closed (updated in the PR title).

arporter

Phew, I've made it. There are a couple of changes that worry me a bit but other than that it's just small things. To cover myself, I have asked (a lot) for modified tests to have compilation checks added where possible.

arporter · 2025-02-05T09:56:34Z

doc/developer_guide/psyir.rst

-``LFRicInvoke``. In both cases these methods make use of the subclasses
-of ``LFRicCollection`` to declare variables.
+the ``stub_declarations`` and ``invoke_declarations`` methods in the
+appropirate ``LFRicCollection``.


"appropriate"

arporter · 2025-02-05T10:04:22Z

src/psyclone/domain/lfric/lfric_cell_iterators.py

        if self._kernel.cma_operation not in ["apply", "matrix-matrix"]:
            for name in self._nlayers_names:
                sym = self.symtab.lookup(name)
+                # Symbols are created thinking for the Invoke context, so make


Thanks for adding. Perhaps: "Some Symbols are created as though for an Invoke context, so make sure they are..."

arporter · 2025-02-05T10:22:47Z

src/psyclone/domain/lfric/lfric_kern.py

@@ -145,6 +148,10 @@ def reference_accesses(self, var_accesses):
        # Use the KernelCallArgList class, which can also provide variable
        # access information:
        create_arg_list = KernCallArgList(self)
+        # KernCallArgList creates symbols (sometimes with wrong type), we don't


You've not added a TODO here?

src/psyclone/domain/lfric/lfric_kern.py

arporter · 2025-02-05T10:27:37Z

src/psyclone/domain/lfric/lfric_kern.py

@@ -469,12 +469,9 @@ def is_intergrid(self):
        return self._intergrid_ref is not None

    @property
-    def colourmap(self):
+    def colourmap(self: DataSymbol):


I think you meant -> DataSymbol:?

arporter · 2025-02-06T16:36:56Z

src/psyclone/tests/psyir/nodes/psy_data_node_test.py

-    PSyData node to add the declaration (which the PSyData node added to the
-    symbol table, but since the symbol table is not used by gen_code, it
-    would otherwise be missing).
+    already be generated by PSyIR).


You've shortened this docstring a bit too much :-)

arporter · 2025-02-06T16:38:28Z

src/psyclone/tests/psyir/nodes/structure_reference_test.py

-    assert ("'symbol' argument to StructureReference.create() should be a "
-            "DataSymbol but found 'NoneType'" in str(err.value))
+    assert ("A StructureReference must refer to a symbol that is (or could be)"
+            " a structure, has been given a 'None' with name: 'unknown'")


"...but has been given a ..."

arporter · 2025-02-06T16:38:48Z

src/psyclone/tests/psyir/nodes/structure_reference_test.py

-    assert ("symbol that is (or could be) a structure, however symbol "
-            "'fake' has type 'Scalar" in str(err.value))
+    assert ("A StructureReference must refer to a symbol that is (or could be)"
+            " a structure, has been given a 'Scalar' with name: 'unknown'")


"..., but has been given a ..."

arporter · 2025-02-06T16:42:26Z

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


-    groups = re.search(correct_re, code, re.I)
-    assert groups is not None
+    # groups = re.search(correct_re, code, re.I)


This needs reinstating?

arporter · 2025-02-06T16:47:17Z

src/psyclone/tests/utilities.py

+                code = str(psy_ast.gen)
+                psy_file.write(fll.process(code))
+
+            # Not everything is captured by PSyIR as Symbols (e.g. multiple


I don't quite understand this comment?

sergisiso added 30 commits April 9, 2024 13:47

#1010 Start converting LFRic PSy-layer declarations and initialisatio…

bc49de6

…ns to PSyIR

Merge remote-tracking branch 'origin/1495_type_bound_methods' into 10…

a3e1b35

…10_remaining_lfric_lowering

#1010 Continue porting LFRic declarations and initialisations to PSyIR

e864bc2

#1010 Pass one more test using backend declarations instead of f2pygen

df68488

#1010 Bring to master

cd515be

#1010 More tests use backend declarations instead of f2pygen

0b90f7b

#1010 More tests use backend declarations instead of f2pygen

96c1aa6

#1010 More tests use backend declarations instead of f2pygen, pass cu…

5c74c42

…rsor on invokes declarations and initialisations

#1010 More tests use backend declarations instead of f2pygen, and add…

e661950

… invoke comments

#1010 More tests use backend declarations instead of f2pygen

64a6bfd

#1010 Update dynamo0p3_basis test to the fortran backend syntax

6c55316

#1010 Start updating stub generation tests for new backend

93f88f3

#1010 Bring to master

77fe7ff

Merge remote-tracking branch 'origin/1010_remaining_lfric_lowering' i…

99ffbbe

…nto 1010_remaining_lfric_lowering

#1010 Fix tests in psyir

99422e6

Merge remote-tracking branch 'origin/master' into 1010_remaining_lfri…

9be02af

…c_lowering

#1010 Fix tests in psyir introduced by lastest update to master

2481022

#1010 Merge master and refactored builtin tests

decd073

#1010 Fix output syntax in gocean opencl trans tests

0e05a0b

#1010 Start updating dynamo0p3_transformations_test to check output w…

ef34fa1

…ith psyir backend

#1010 Update more dynamo0p3_transformations_test test to use the psyi…

62e41e5

…r backend

#1010 Update more dynamo0p3_transformations_test test to use the psyi…

8908239

…r backend

#1010 Bring to master

930ddb9

#1010 Fix some issues introduced due to previous merge with master

a2942f1

#1010 Fix issues with reference_element

cf63302

#1010 Start porting reductions to generic PSyIR

3d8969c

#1010 Remove file mistakenly introduced when brining the branch to ma…

98a899c

…ster

#1010 Move LFRic reductions to OpenMP classes

ddef4de

#1010 Bring to master

b697236

#1010 Modifications to pass more LFRic OpenMP reduction tests

6ab2c8c

sergisiso changed the title ~~(closes #1010) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen)~~ (closes #1010, #1910) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) Jan 27, 2025

sergisiso changed the title ~~(closes #1010, #1910) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen)~~ (closes #1010, #1910, #719) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) Jan 27, 2025

sergisiso added 7 commits January 27, 2025 13:32

#1010 Improve naming, docstrings and comments

92fed41

#1010 Clean up LFRicLoop code

bb53018

#1010 Refactor LFRicCollections.declarations

1a06860

#1010 Clean up PR

cdb2399

#1010 Fix flake8 issues

0614d93

#1010 Bring to master

d7fcd56

#1010 Fix typing issue with python3.8

1a67f95

sergisiso added 2 commits January 30, 2025 14:02

#1010 Remove cursor argument from invoke_declarations

5d66448

#1010 Improve tests and documentation

ac88bd0

sergisiso temporarily deployed to integration January 30, 2025 16:35 — with GitHub Actions Inactive

#1010 Remove leftover f2pygen in gocean and updated test syntax

6ac84f3

sergisiso changed the title ~~(closes #1010, #1910, #719) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen)~~ (closes #1010, #1910, #719, #798) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) Jan 31, 2025

sergisiso added 5 commits January 31, 2025 17:22

#1010 Fix compilation tests

6cef2df

#1010 Remove remaining references to the gen_code method

fe26a76

#1010 Rename gen_code variable to code in tests

7040b7c

#1010 Delete f2pygen classes

5996da2

#1010 Clean up f2pygen mentions and fix some tests/coverage

8db5dfa

sergisiso temporarily deployed to integration February 3, 2025 11:47 — with GitHub Actions Inactive

sergisiso changed the title ~~(closes #1010, #1910, #719, #798) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen)~~ (closes #1010, #1910, #719, #798, #1648) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) Feb 3, 2025

sergisiso added ready for review and removed reviewed with actions labels Feb 3, 2025

arporter added under review and removed ready for review labels Feb 5, 2025

arporter requested changes Feb 6, 2025

View reviewed changes

arporter added reviewed with actions and removed under review labels Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(closes #1010, #1910, #719, #798, #1648) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) #2834

(closes #1010, #1910, #719, #798, #1648) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) #2834

sergisiso commented Dec 29, 2024

sergisiso commented Jan 30, 2025

sergisiso commented Feb 3, 2025

arporter left a comment

arporter Feb 5, 2025

arporter Feb 5, 2025

arporter Feb 5, 2025

arporter Feb 5, 2025

arporter Feb 6, 2025

arporter Feb 6, 2025

arporter Feb 6, 2025

arporter Feb 6, 2025

arporter Feb 6, 2025

(closes #1010, #1910, #719, #798, #1648) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) #2834

Are you sure you want to change the base?

(closes #1010, #1910, #719, #798, #1648) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) #2834

Conversation

sergisiso commented Dec 29, 2024

sergisiso commented Jan 30, 2025

sergisiso commented Feb 3, 2025

arporter 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