Merge remote-tracking branch 'origin/master' into single_nemo_script

Author: sergisiso

changelog

@@ -1,3 +1,6 @@
+   14) PR #3254 for #3253. Fix openmp parallel lowering with kernels and
+   improve dependency messages
+
    13) PR #3239 for #3199. Add OMPCritical Directive and Transformation.
 
    12) PR #3226 for #514. Refactor LFRic reductions to use OMPReductionClause.

src/psyclone/core/access_sequence.py

@@ -45,9 +45,8 @@
 
 from psyclone.core.access_type import AccessType
 from psyclone.core.signature import Signature
-
-
 from psyclone.errors import InternalError
+
 if TYPE_CHECKING:  # pragma: no cover
     from psyclone.psyir.nodes import Node
     from psyclone.psyir.symbols import Symbol
@@ -157,14 +156,19 @@ def description(self) -> str:
                   messages.
         '''
         # pylint: disable=import-outside-toplevel
-        from psyclone.psyir.nodes import Statement
+        from psyclone.psyir.nodes import Assignment
         from psyclone.psyir.symbols import Symbol
         if isinstance(self.node, Symbol):
             text = f"the definition of Symbol '{self.node}'"
         else:
-            stmt = self.node.ancestor(Statement, include_self=True)
-            if stmt:
-                text = f"'{stmt.debug_string()}'"
+            from psyclone.psyGen import CodedKern
+            kernel = self.node.ancestor(CodedKern, include_self=True)
+            stmt = self.node.ancestor(Assignment, include_self=True)
+            if kernel:
+                text = f"'{self.node.debug_string()}' (inside '{kernel.name})'"
+            elif stmt:
+                text = (f"'{self.node.debug_string()}' "
+                        f"in '{stmt.debug_string().strip()}'")
             else:
                 text = f"'{self.node.debug_string()}'"
         return text

src/psyclone/psyir/nodes/omp_directives.py

@@ -1353,19 +1353,18 @@ def lower_to_language_level(self) -> Node:
             )
             self.dir_body.addchild(assignment, 0)
 
-        # Lower the first two children
-        for child in self.children[:2]:
-            child.lower_to_language_level()
-
-        # Create data sharing clauses (order alphabetically to make generation
-        # reproducible)
+        # Create data sharing clauses (before lowering, so the CodedKern
+        # semantics are taken into account)
         private, fprivate, need_sync = self.infer_sharing_attributes()
+
+        # Order the clause variables alphabetically (to facilitate comparisons)
         if reprod_red_call_list:
             private.add(thread_idx)
         private_clause = OMPPrivateClause.create(
                             sorted(private, key=lambda x: x.name))
         fprivate_clause = OMPFirstprivateClause.create(
                             sorted(fprivate, key=lambda x: x.name))
+
         # Check all of the need_sync nodes are synchronized in children.
         # unless it has reduction_kernels which are handled separately
         sync_clauses = self.walk(OMPDependClause)
@@ -1390,10 +1389,14 @@ def lower_to_language_level(self) -> Node:
         self.children[2].replace_with(private_clause)
         self.children[3].replace_with(fprivate_clause)
 
+        # Continue lowering children (but not the clauses we just added)
+        for child in self.children[:2]:
+            child.lower_to_language_level()
+
+        # Now finish the LFRic reductions (which need lowering first)
         if reduction_kernels and not reprod_red_call_list:
-            self._add_reduction_clauses(need_sync=need_sync)
+            self._add_reduction_clauses()
 
-        # Now finish the reproducible reductions
         for call in reversed(reprod_red_call_list):
             call.reduction_sum_loop(self.parent, self.position,
                                     self.scope.symbol_table)

src/psyclone/psyir/tools/dependency_tools.py

@@ -109,7 +109,7 @@ def __init__(self, message, code, var_names=None, var_infos=None):
     # ------------------------------------------------------------------------
     def __str__(self):
         if len(self._var_names) == 0:
-            return self._message
+            return str(self._message)
         if len(self._var_names) == 1:
             return f"{self._message} Variable: '{self._var_names[0].strip()}'."
         return f"{self._message} Variables: {self._var_names}."
@@ -227,8 +227,9 @@ def _add_message(self, message, code, var_names=None, var_infos=None):
                               "The var_infos argument to _add_message must "
                               "be a list of Signature/AccessSequence pairs")
 
-        self._messages.append(Message(f"{message_type}: {message}", code,
-                                      var_names, var_infos))
+        self._messages.append(Message(
+                LazyString(lambda: f"{message_type}: {message}"),
+                code, var_names, var_infos))
 
     # -------------------------------------------------------------------------
     def get_all_messages(self):
@@ -738,37 +739,35 @@ def _array_access_parallelisable(self, loop_variables, var_info):
                 if not self._is_loop_carried_dependency(loop_variables,
                                                         write_access,
                                                         other_access):
+                    # We can capture the loop variable 'write_access' and
+                    # 'other_access' in the lambdas because we immidiately
+                    # return after creating the lambdas, not allowing the
+                    # variables to be redefined anymore.
+                    # pylint: disable=cell-var-from-loop
+
                     # There is a dependency. Try to give precise error
                     # messages:
                     if write_access is other_access:
                         # The write access has a dependency on itself, e.g.
                         # a(3) = ...    or a((i-2)**2) = ...
                         # Both would result in a write-write conflict
-                        node = write_access.node
-                        self._add_message(
-                            f"The write access to '{var_info.var_name}' in "
-                            f"'{node.debug_string()}' causes "
-                            f"a write-write race condition.",
+                        self._add_message(LazyString(
+                            lambda:
+                                (f"The write access to "
+                                 f"{write_access.description} "
+                                 f"causes a write-write race condition.")),
                             DTCode.ERROR_WRITE_WRITE_RACE,
                             [var_info.var_name])
                     else:
                         # Get 'read' or 'write' etc
                         access_type = str(other_access.access_type).lower()
 
-                        # We need to use default parameters for wnode and
-                        # onode, since otherwise the value of a variable might
-                        # be different when the message is actually evaluated.
-                        # Some pylint version complain here (because of the
-                        # above). The code is correct, so disable this
-                        # message:
-                        # pylint: disable=cell-var-from-loop
                         self._add_message(LazyString(
-                            lambda wnode=write_access.node,
-                            onode=other_access.node:
+                            lambda:
                                 (f"The write access to "
-                                 f"'{wnode.debug_string().strip()}' and the "
+                                 f"{write_access.description} and the "
                                  f"{access_type} access to "
-                                 f"'{onode.debug_string().strip()}' "
+                                 f"{other_access.description} "
                                  f"are dependent and cannot be "
                                  f"parallelised.")),
                             DTCode.ERROR_DEPENDENCY,

src/psyclone/tests/core/access_sequence_test.py

@@ -47,6 +47,7 @@
 from psyclone.psyir.nodes import (
     Assignment, Node, Reference, Return, ArrayReference)
 from psyclone.psyir.symbols import DataSymbol, INTEGER_TYPE, Symbol
+from psyclone.psyGen import CodedKern
 
 
 def test_access_info() -> None:
@@ -141,6 +142,26 @@ def test_access_info_description() -> None:
     assert ("definition of symbol 'test: datasymbol<scalar" in
             ainfo.description.lower())
 
+    # Assignment has a special description to provide more context
+    ref1 = Reference(osym)
+    ref2 = Reference(asym)
+    assign = Assignment.create(ref1, ref2)
+    ainfo = AccessInfo(AccessType.READ, ref2)
+    assert "'test' in 'something = test'" in ainfo.description
+
+    # CodedKernel has a special description to provide more context
+    class MockKern(CodedKern):
+        ''' Create a API agnostic subclass of CodedKern '''
+        name = "kernel_name"
+
+        def __init__(self):
+            Node.__init__(self)
+
+    mk = MockKern()
+    mk._children = [assign]
+    assign._parent = mk
+    assert "'test' (inside 'kernel_name)'" in ainfo.description
+
 
 # -----------------------------------------------------------------------------
 def test_variable_access_sequence() -> None:

src/psyclone/tests/domain/gocean/transformations/gocean1p0_transformations_test.py

@@ -43,6 +43,7 @@
 import pytest
 from psyclone.configuration import Config
 from psyclone.domain.gocean.transformations import GOceanLoopFuseTrans
+from psyclone.domain.common.transformations import KernelModuleInlineTrans
 from psyclone.errors import GenerationError
 from psyclone.gocean1p0 import GOKern
 from psyclone.parse import ModuleManager
@@ -103,7 +104,7 @@ def test_loop_fuse_error():
     assert "Both nodes must be of the same GOLoop class." in str(err.value)
 
 
-def test_omp_parallel_loop(tmpdir, fortran_writer):
+def test_omp_paralleldo_loop(tmpdir, fortran_writer):
     '''Test that we can generate an OMP PARALLEL DO correctly,
     independent of whether or not we are generating constant loop bounds '''
     psy, invoke = get_invoke("single_invoke_three_kernels.f90", API, idx=0,
@@ -176,7 +177,7 @@ def test_omp_region_with_single_loop(tmpdir):
     within_omp_region = False
     call_count = 0
     for line in gen.split('\n'):
-        if '!$omp parallel' in line:
+        if '!$omp parallel default(shared) private(i,j)' in line:
             within_omp_region = True
         if '!$omp end parallel' in line:
             within_omp_region = False
@@ -192,7 +193,7 @@ def test_omp_region_with_single_loop(tmpdir):
     within_omp_region = False
     call_count = 0
     for line in gen.split('\n'):
-        if '!$omp parallel' in line:
+        if '!$omp parallel default(shared) private(i,j)' in line:
             within_omp_region = True
         if '!$omp end parallel' in line:
             within_omp_region = False
@@ -222,7 +223,7 @@ def test_omp_region_with_slice(tmpdir):
     within_omp_region = False
     call_count = 0
     for line in gen.split('\n'):
-        if '!$omp parallel' in line:
+        if '!$omp parallel default(shared) private(i,j)' in line:
             within_omp_region = True
         if '!$omp end parallel' in line:
             within_omp_region = False
@@ -288,7 +289,7 @@ def test_omp_region_no_slice(tmpdir):
     within_omp_region = False
     call_count = 0
     for line in gen.split('\n'):
-        if '!$omp parallel' in line:
+        if '!$omp parallel default(shared) private(i,j)' in line:
             within_omp_region = True
         if '!$omp end parallel' in line:
             within_omp_region = False
@@ -319,7 +320,7 @@ def test_omp_region_no_slice_const_bounds(tmpdir):
     within_omp_region = False
     call_count = 0
     for line in gen.split('\n'):
-        if '!$omp parallel' in line:
+        if '!$omp parallel default(shared) private(i,j)' in line:
             within_omp_region = True
         if '!$omp end parallel' in line:
             within_omp_region = False
@@ -452,6 +453,14 @@ def test_omp_region_retains_kernel_order3(tmpdir):
 
     # Kernels should be in order {compute_cu, compute_cv, time_smooth}
     assert cu_idx < cv_idx < ts_idx
+
+    # Check that the two directive are different statements in above the
+    # second loop (iterates over cv_fld internal) and that the private
+    # clause (now on the parallel directive) only has i and j.
+    assert ("!$omp parallel default(shared) private(i,j)\n"
+            "    !$omp do schedule(static)\n"
+            "    do j = cv_fld%internal%ystart" in gen)
+
     assert GOceanBuild(tmpdir).code_compiles(psy)
 
 
@@ -482,7 +491,7 @@ def test_omp_region_before_loops_trans(tmpdir):
     omp_region_idx = -1
     omp_do_idx = -1
     for idx, line in enumerate(gen.split('\n')):
-        if '!$omp parallel' in line:
+        if '!$omp parallel default(shared) private(i,j)' in line:
             omp_region_idx = idx
         if '!$omp do' in line:
             omp_do_idx = idx
@@ -502,6 +511,11 @@ def test_omp_region_after_loops_trans(tmpdir):
                              dist_mem=False)
     schedule = invoke.schedule
 
+    # We test with inlining because in the past we had an error when
+    # producing the clauses if the calls were inlined.
+    for kern in schedule.kernels():
+        KernelModuleInlineTrans().apply(kern)
+
     # Put an OpenMP do directive around each loop contained
     # in the schedule
     ompl = GOceanOMPLoopTrans()
@@ -519,7 +533,7 @@ def test_omp_region_after_loops_trans(tmpdir):
     omp_region_idx = -1
     omp_do_idx = -1
     for idx, line in enumerate(gen.split('\n')):
-        if '!$omp parallel' in line:
+        if '!$omp parallel default(shared) private(i,j)' in line:
             omp_region_idx = idx
         if '!$omp do' in line:
             omp_do_idx = idx

src/psyclone/tests/domain/gocean/transformations/gocean_move_iteration_boundaries_inside_kernel_trans_test.py

@@ -207,7 +207,7 @@ def test_go_move_iteration_boundaries_inside_kernel_two_kernels_apply_twice(
   ystop_1 = SIZE(uold_fld%data, dim=2)
   do j = 1, SIZE(uold_fld%data, dim=2), 1
     do i = 1, SIZE(uold_fld%data, dim=1), 1
-      call time_smooth_code(i, j, u_fld%data, unew_fld%data, uold_fld%data, \
+      call time_smooth_code(i, j, cu_fld%data, unew_fld%data, uold_fld%data, \
 xstart_1, xstop_1, ystart_1, ystop_1)
     enddo
   enddo

src/psyclone/tests/domain/gocean/transformations/gocean_opencl_trans_test.py

@@ -1047,7 +1047,7 @@ def test_set_kern_args(kernel_outputdir):
 
     # There is also only one version of the set_args for the second kernel
     assert generated_code.count("subroutine time_smooth_code_set_args("
-                                "kernel_obj, u_fld, unew_fld, uold_fld, "
+                                "kernel_obj, cu_fld, unew_fld, uold_fld, "
                                 "xstart_1, xstop_1, ystart_1, ystop_1)") == 1
     assert GOceanOpenCLBuild(kernel_outputdir).code_compiles(psy)
 

src/psyclone/tests/gocean1p0_test.py

@@ -155,7 +155,7 @@ def test_two_kernels(tmpdir, dist_mem):
     second_kernel = (
         "    do j = 1, SIZE(uold_fld%data, dim=2), 1\n"
         "      do i = 1, SIZE(uold_fld%data, dim=1), 1\n"
-        "        call time_smooth_code(i, j, u_fld%data, unew_fld%data, "
+        "        call time_smooth_code(i, j, cu_fld%data, unew_fld%data, "
         "uold_fld%data)\n"
         "      enddo\n"
         "    enddo\n\n"