Merge pull request #3181 from stfc/3173_quickfix

(closes #3173) Add protections for CodeBlocks in the data_sharing_attribute_mixin

Author: arporter

changelog

@@ -1,3 +1,6 @@
+   7) PR #3181 for #3173. Adds protections for CodeBlocks in the
+   DataSharingAttributeMixin.   
+
    6) PR #3174 and #3176 towards #2668. Add kwargs to more transformations.
 
    5) PR #3186 for #3185. Small fix to SymbolicMaths.expand for Boolean

src/psyclone/psyir/nodes/data_sharing_attribute_mixin.py

@@ -44,6 +44,7 @@
 from psyclone.psyir.nodes.loop import Loop
 from psyclone.psyir.nodes.while_loop import WhileLoop
 from psyclone.psyir.nodes.omp_clauses import OMPReductionClause
+from psyclone.psyir.nodes.reference import Reference
 from psyclone.psyir.symbols import DataSymbol, Symbol
 
 
@@ -134,26 +135,42 @@ def infer_sharing_attributes(self) -> \
                 continue
             symbol = accesses[0].node.scope.symbol_table.lookup(
                 name, otherwise=None)
+            if symbol is None:
+                # The signature does not match any symbol! This is probably a
+                # structure, we will consider them shared
+                continue
+
+            # A parallel loop variable is always private
+            if (isinstance(self.dir_body[0], Loop) and
+                    self.dir_body[0].variable is symbol):
+                private.add(symbol)
+                continue
 
             # If it is manually marked as a local symbol, add it to private or
             # firstprivate set
             if (isinstance(symbol, DataSymbol) and
                     isinstance(self.dir_body[0], Loop) and
                     symbol in self.dir_body[0].explicitly_private_symbols):
                 visited = set()
-                if (
-                    # The loop variable is always private
-                    self.dir_body[0].variable is not symbol and
-                    # For anything else, if it uses a value coming from before
-                    # the loop scope, make it firstprivate
-                    any(access.node.enters_scope(self, visited_nodes=visited)
-                        for access in accesses)):
-                    fprivate.add(symbol)
+                for access in accesses:
+                    if not isinstance(access.node, Reference):
+                        # Nodes that are not References do not have
+                        # 'enters_scope', so the analysis below can't be
+                        # done and we defensively use 'firstprivate'
+                        # TODO #3124: Remove this special-case
+                        fprivate.add(symbol)
+                        break
+                    if access.node.enters_scope(self, visited):
+                        # If it uses a value coming from before the loop
+                        # scope, make it 'firstprivate'
+                        fprivate.add(symbol)
+                        break
                 else:
+                    # Everything else can be just 'private'
                     private.add(symbol)
                 continue
 
-            # All arrays not explicitly marked as threadprivate are shared
+            # All arrays not explicitly marked as explicitly_private are shared
             if any(accs.has_indices() for accs in accesses):
                 continue
 
@@ -177,6 +194,7 @@ def infer_sharing_attributes(self) -> \
 
             has_been_read = False
             last_read_position = 0
+            visited_nodes = set()  # To avoid enters_scope repetitions
             for access in accesses:
                 if access.is_any_read():
                     has_been_read = True
@@ -195,19 +213,13 @@ def infer_sharing_attributes(self) -> \
                         limit=self,
                         include_self=True)
                     if not loop_ancestor:
-                        # If we find it at least once outside a loop we keep it
-                        # as shared
+                        # If we find it at least one WRITE outside a loop we
+                        # keep it as shared
                         break
 
                     # Otherwise, the assignment to this variable is inside a
                     # loop (and it will be repeated for each iteration), so
-                    # we declare it as private or need_sync
-                    name = signature.var_name
-                    # TODO #2094: var_name only captures the top-level
-                    # component in the derived type accessor. If the attributes
-                    # only apply to a sub-component, this won't be captured
-                    # appropriately.
-                    symbol = access.node.scope.symbol_table.lookup(name)
+                    # we declare it as [first]private or need_sync
 
                     # If it has been read before we have to check if ...
                     if has_been_read:
@@ -220,23 +232,31 @@ def infer_sharing_attributes(self) -> \
                             need_sync.add(symbol)
                         break
 
-                    # If the write is not guaranteed, we make it firstprivate
-                    # so that in the case that the write doesn't happen we keep
-                    # the original value
+                    # If the write is not guaranteed (because it is inside a
+                    # conditional), make it firstprivate to keep the original
+                    # value when the branch is never taken. Unless this never
+                    # had a value before the loop (as some compilers don't
+                    # like uninitialised firstprivates)
                     conditional_write = access.node.ancestor(
                         IfBlock,
                         limit=loop_ancestor,
                         include_self=True)
                     if conditional_write:
-                        # If it's used before the loop, make it firstprivate
-                        visited = set()
-                        if any(access.node.enters_scope(
-                                                self, visited_nodes=visited)
-                               for access in accesses):
-                            fprivate.add(symbol)
-                            break
-                    # Otherwise it is just private
-                    private.add(symbol)
+                        # Check if it gets a value from before the loop
+                        for access in accesses:
+                            if not isinstance(access.node, Reference):
+                                # Nodes that are not References do not have
+                                # 'enters_scope', so the analysis below can't
+                                # be done and we defensively use 'firstprivate'
+                                # TODO #3124: Remove this special-case
+                                fprivate.add(symbol)
+                                break
+                            if access.node.enters_scope(self, visited_nodes):
+                                fprivate.add(symbol)
+                                break
+                    # Otherwise it is just 'private'
+                    if symbol not in fprivate:
+                        private.add(symbol)
                     break
 
         return private, fprivate, need_sync

src/psyclone/psyir/transformations/parallel_loop_trans.py

@@ -374,7 +374,7 @@ def apply(self, node, options=None, verbose: bool = False,
         '''
         if not options:
             self.validate_options(
-                    verbose=verbose, collapse=collapse,
+                    verbose=verbose, collapse=collapse, force=force,
                     ignore_dependencies_for=ignore_dependencies_for,
                     privatise_arrays=privatise_arrays,
                     sequential=sequential, nowait=nowait,
@@ -401,7 +401,7 @@ def apply(self, node, options=None, verbose: bool = False,
             reduction_ops = options.get("reduction_ops", [])
 
         self.validate(node, options=options, verbose=verbose,
-                      collapse=collapse,
+                      collapse=collapse, force=force,
                       ignore_dependencies_for=ignore_dependencies_for,
                       privatise_arrays=privatise_arrays,
                       sequential=sequential, nowait=nowait,

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

@@ -676,6 +676,48 @@ def test_infer_sharing_attributes_with_explicitly_private_symbols(
     assert "scalar2" in [x.name for x in fpvars]
 
 
+def test_infer_sharing_attributes_with_codeblocks(
+        fortran_reader, fortran_writer):
+    ''' Tests the infer_sharing_attributes() method when some of the loops have
+    Codeblocks inside it. We check that the infer_sharing attribute analysis
+    succeed by assuming worst case.
+    '''
+    psyir = fortran_reader.psyir_from_source('''
+        subroutine my_subroutine()
+            use other, only: mystruct
+            integer :: i, j, scalar1 = 1, scalar2 = 2
+            real, dimension(10) :: array, array2
+            write(*,*) scalar1, scalar2
+            do j = 1, 10
+               do i = 1, size(array, 1)
+                   write(*,*) scalar2, mystruct(i)%field2
+                   if (.true.) then
+                       scalar1 = 1
+                       write(*,*) scalar1, mystruct(i)%field1
+                   end if
+                   scalar2 = scalar1 + 1
+                   write(*,*) scalar1, scalar2
+               enddo
+            enddo
+            write(*,*) scalar1, scalar2
+        end subroutine''')
+    omplooptrans = OMPLoopTrans()
+    omplooptrans.omp_directive = "paralleldo"
+    loop = psyir.walk(Loop)[0]
+    # Make sure that the write statements inside the loop are CodeBlocks,
+    # otherwise we need a new test example
+    assert loop.has_descendant(nodes.CodeBlock)
+    loop.explicitly_private_symbols.add(
+            loop.scope.symbol_table.lookup("scalar2"))
+    omplooptrans.apply(loop, node_type_check=False, force=True)
+
+    # Here we mostly check that the infer_sharing attributes doesn't fall
+    # over with CodeBlocks. This will often still be defensively firstprivate
+    # as we condiser all codeblock accesses as READWRITE.
+    output = fortran_writer(psyir)
+    assert "firstprivate(scalar1,scalar2)" in output
+
+
 def test_infer_sharing_attributes(fortran_reader):
     ''' Tests for the infer_sharing_attributes() method of OpenMP directives
     with generic code inside the directive body.