Updates for inlining with common variables

src/psyclone/psyir/symbols/symbol_table.py

@@ -48,6 +48,7 @@
 import inspect
 import copy
 import logging
+import re
 from typing import Any, List, Optional, Union, TYPE_CHECKING
 
 from psyclone.configuration import Config
@@ -57,6 +58,7 @@
     ImportInterface, RoutineSymbol, Symbol, SymbolError, UnresolvedInterface)
 from psyclone.psyir.symbols.intrinsic_symbol import IntrinsicSymbol
 from psyclone.psyir.symbols.typed_symbol import TypedSymbol
+from psyclone.psyir.symbols.datatypes import UnsupportedFortranType
 
 if TYPE_CHECKING:
     from psyclone.psyir.nodes.scoping_node import ScopingNode
@@ -704,6 +706,12 @@ def check_for_clashes(self, other_table, symbols_to_skip=()):
                     isinstance(other_sym, IntrinsicSymbol)):
                 continue
 
+            # If both symbols have CommonBlockInterface, they represent the
+            # same shared COMMON-block data. They cannot (and do not need to)
+            # be renamed, so treat this as a benign clash.
+            if this_sym.is_commonblock and other_sym.is_commonblock:
+                continue
+
             if other_sym.is_import and this_sym.is_import:
                 # Both symbols are imported. That's fine as long as they have
                 # the same import interface (are imported from the same
@@ -945,13 +953,34 @@ def _add_symbols_from_table(self, other_table, symbols_to_skip=()):
             already been updated to refer to a Container in this table.
 
         '''
+
         for old_sym in other_table.symbols:
 
             if old_sym in symbols_to_skip or isinstance(old_sym,
                                                         ContainerSymbol):
                 # We've dealt with Container symbols in _add_container_symbols.
                 continue
 
+            # Avoid duplicate COMMON-block marker symbols when multiple
+            # routines sharing the same COMMON blocks are inlined into a
+            # single caller.  Each routine is parsed independently so its
+            # _PSYCLONE_INTERNAL_COMMONBLOCK_N markers carry different
+            # numbers and may therefore never trigger the name-clash path;
+            # we must scan *all* existing markers in self for an identical
+            # declaration before attempting to add.
+            if (self._normalize(old_sym.name).startswith(
+                    "_psyclone_internal_commonblock")
+                    and isinstance(old_sym.datatype, UnsupportedFortranType)):
+                if any(
+                    sym.datatype.declaration == old_sym.datatype.declaration
+                    for sym in self.symbols
+                    if (self._normalize(sym.name).startswith(
+                            "_psyclone_internal_commonblock")
+                        and isinstance(sym.datatype,
+                                       UnsupportedFortranType))
+                ):
+                    continue
+
             try:
                 self.add(old_sym)
 
@@ -1000,11 +1029,45 @@ def _handle_symbol_clash(self, old_sym, other_table):
 
         self_sym = self.lookup(old_sym.name)
         if old_sym.is_unresolved and self_sym.is_unresolved:
+            # Update after fixing issue #3392
             # The clashing symbols are both unresolved so we ASSUME that
             # check_for_clashes has previously determined that they must
             # refer to the same thing and we don't have to do anything.
             return
 
+        if old_sym.is_commonblock and self_sym.is_commonblock:
+            return
+
+        if (isinstance(old_sym.datatype, UnsupportedFortranType) and
+                isinstance(self_sym.datatype, UnsupportedFortranType)):
+            if old_sym.datatype.declaration == self_sym.datatype.declaration:
+                # Identical COMMON-block markers – already present in self.
+                return
+            # Markers have different declarations.  Skip the incoming one only
+            # if its COMMON-block name(s) overlap with those already in self:
+            # that means the block is already declared and adding a second
+            # marker for it would produce a "Symbol X is already in a COMMON
+            # block" compile error.  If the block names are different this is
+            # a genuinely new COMMON block and we fall through to the
+            # rename-and-add path below.
+            if self._normalize(old_sym.name).startswith(
+                    "_psyclone_internal_commonblock"):
+                _blk_re = re.compile(r"/\s*(\w*)\s*/", re.IGNORECASE)
+                old_blocks = set(_blk_re.findall(
+                    old_sym.datatype.declaration))
+                # Check ALL existing commonblock markers in self, not just
+                # the same-named one, because the numbering may differ when
+                # the caller already has extra COMMON blocks of its own.
+                for sym in self.symbols:
+                    if (self._normalize(sym.name).startswith(
+                            "_psyclone_internal_commonblock")
+                            and isinstance(sym.datatype,
+                                           UnsupportedFortranType)):
+                        self_blocks = set(_blk_re.findall(
+                            sym.datatype.declaration))
+                        if old_blocks & self_blocks:
+                            return
+
         # A Symbol with the same name already exists so we attempt to rename
         # first the one that we are adding and failing that, the existing
         # symbol in this table.

src/psyclone/psyir/transformations/inline_trans.py

@@ -147,6 +147,7 @@ def apply(self,
               use_first_callee_and_no_arg_check: bool = False,
               permit_codeblocks: bool = False,
               permit_unsupported_type_args: bool = False,
+              parameter_cloning: bool = True,
               **kwargs
               ):
         '''
@@ -163,6 +164,13 @@ def apply(self,
             if the target routine contains a CodeBlock.
         :param permit_unsupported_type_args: If `True` then the target routine
             is permitted to have arguments of UnsupportedType.
+        :param parameter_cloning: if `True` (the default), constant
+            (PARAMETER) symbols from the routine being inlined are always
+            copied into the call-site symbol table, potentially being renamed
+            to avoid clashes. If `False`, a constant from the routine is
+            skipped when an identical constant (same name, same type, and same
+            value) already exists at the call site, so no duplicate is
+            created.
 
         :raises InternalError: if the merge of the symbol tables fails.
             In theory this should never happen because validate() should
@@ -219,6 +227,15 @@ def apply(self,
         #     just delete the if statement.
         self._optional_arg_eliminate_ifblock_if_const_condition(routine)
 
+        # If parameter_cloning is disabled, identify duplicate constant
+        # (PARAMETER) symbols and redirect their references *before* the
+        # routine body is extracted, so that the extracted statements already
+        # carry references to the call-site symbols.
+        extra_skip: List[DataSymbol] = []
+        if not parameter_cloning:
+            extra_skip = self._redirect_duplicate_parameters(
+                table, routine, routine_table)
+
         # Construct lists of the nodes that will be inserted and all of the
         # References that they contain.
         new_stmts = []
@@ -231,7 +248,8 @@ def apply(self,
         # call site. This preserves any references to them.
         try:
             table.merge(routine_table,
-                        symbols_to_skip=routine_table.argument_list[:])
+                        symbols_to_skip=routine_table.argument_list[:]
+                        + extra_skip)
         except SymbolError as err:
             raise InternalError(
                 f"Error copying routine symbols to call site. This should "
@@ -329,6 +347,116 @@ def apply(self,
                 idx += 1
                 parent.addchild(child, idx)
 
+    def _redirect_duplicate_parameters(
+        self,
+        table,
+        routine: Routine,
+        routine_table,
+    ) -> List[DataSymbol]:
+        '''
+        Identifies constant (PARAMETER) symbols in ``routine_table`` that
+        are identical to constants already present in ``table`` (same name,
+        same type, and same initial value). For each such symbol, every
+        :py:class:`~psyclone.psyir.nodes.Reference` to it inside ``routine``
+        and inside the datatypes / initial-value expressions of other symbols
+        in ``routine_table`` is redirected to point to the corresponding
+        symbol in ``table``.
+
+        Only constants whose initial value is represented as a PSyIR node
+        (i.e. ``initial_value is not None``) are considered; constants of
+        ``UnsupportedFortranType`` with an embedded value string are left
+        unchanged.
+
+        A constant is only considered a duplicate when every routine-local
+        symbol referenced inside its initial-value expression is itself a
+        confirmed duplicate.  This prevents false positives for expressions
+        like ``negflag = .NOT. flag`` when ``flag`` has different values in
+        the caller and the callee (the names would match but the semantics
+        would differ).
+
+        :param table: the call-site symbol table.
+        :type table: :py:class:`psyclone.psyir.symbols.SymbolTable`
+        :param routine: the (copy of the) routine being inlined.
+        :type routine: :py:class:`psyclone.psyir.nodes.Routine`
+        :param routine_table: the symbol table of the routine copy.
+        :type routine_table: :py:class:`psyclone.psyir.symbols.SymbolTable`
+
+        :returns: the list of routine symbols that are duplicates of
+            call-site constants and should be excluded from the subsequent
+            table merge.
+        :rtype: List[:py:class:`psyclone.psyir.symbols.DataSymbol`]
+
+        '''
+        # The names of all local data symbols in the routine table (used to
+        # identify references that point to routine-local constants).
+        routine_local_names = {
+            s.name.lower() for s in routine_table.datasymbols
+            if not s.is_argument
+        }
+
+        # First pass: collect all constants from the routine whose name,
+        # datatype, and initial-value tree match a constant in the call-site
+        # table. The structural comparison uses __eq__, which compares
+        # Reference nodes by symbol name. This is correct for leaf constants
+        # (Literals) and is refined for dependent constants in the second
+        # pass below.
+        candidates: dict = {}
+        for rsym in routine_table.datasymbols:
+            if not rsym.is_constant or rsym.initial_value is None:
+                # Skip constants whose value is not represented as a PSyIR
+                # node (e.g. UnsupportedFortranType with embedded value).
+                continue
+            tsym = table.lookup(rsym.name, otherwise=None)
+            if not isinstance(tsym, DataSymbol):
+                continue
+            if not tsym.is_constant or tsym.initial_value is None:
+                continue
+            if rsym.datatype != tsym.datatype:
+                continue
+            if rsym.initial_value != tsym.initial_value:
+                continue
+            candidates[rsym.name.lower()] = rsym
+
+        # Second pass: iteratively remove candidates whose initial-value
+        # expression references a routine-local symbol that is NOT itself
+        # a confirmed duplicate. Without this step, an expression like
+        # ``negflag = .NOT. flag`` would compare as equal by name even when
+        # ``flag`` has different values in the two routines.
+        changed = True
+        while changed:
+            changed = False
+            to_remove = [
+                name for name, rsym in candidates.items()
+                if any(
+                    dep.name.lower() in routine_local_names
+                    and dep.name.lower() not in candidates
+                    for dep in rsym.initial_value.get_all_accessed_symbols()
+                )
+            ]
+            for name in to_remove:
+                del candidates[name]
+            if to_remove:
+                changed = True
+
+        duplicates: List[DataSymbol] = list(candidates.values())
+
+        # Redirect all references from duplicate routine symbols to their
+        # call-site counterparts.
+        for rsym in duplicates:
+            tsym = table.lookup(rsym.name)
+            # Update all References in the routine body.
+            routine.replace_symbols_using(tsym)
+            # Update any references to rsym embedded in the datatypes or
+            # initial-value expressions of other symbols in routine_table.
+            for sym in routine_table.symbols:
+                if sym is rsym:
+                    continue
+                sym.replace_symbols_using(tsym)
+                if hasattr(sym, 'datatype') and sym.datatype is not None:
+                    sym.datatype.replace_symbols_using(tsym)
+
+        return duplicates
+
     def _optional_arg_resolve_present_intrinsics(self,
                                                  routine_node: Routine,
                                                  arg_match_list: List = []):

src/psyclone/tests/psyir/backend/fortran_common_block_test.py

@@ -61,6 +61,8 @@ def test_fw_common_blocks(fortran_reader, fortran_writer, tmpdir):
     routine = psyir.walk(Routine)[0]
 
     assert routine.symbol_table.lookup("a").is_commonblock  # Sanity check
+    assert routine.symbol_table.lookup("d").is_commonblock  # Sanity check
+    assert routine.symbol_table.lookup("e").is_commonblock  # Sanity check
 
     code = fortran_writer(routine)
     assert code == (

src/psyclone/tests/psyir/symbols/symbol_table_test.py

@@ -1325,6 +1325,90 @@ def test_handle_symbol_clash_imported_symbols():
             "of the same name imported from 'Ridcully'" in str(err.value))
 
 
+def test_handle_symbol_clash_commonblock_same_declaration():
+    '''Test that _handle_symbol_clash() ignores duplicate COMMON-block
+    markers with identical declarations.'''
+    table1 = symbols.SymbolTable()
+    table2 = symbols.SymbolTable()
+    decl = "common /keep_me/ a"
+    marker_name = "_PSYCLONE_INTERNAL_COMMONBLOCK_1"
+    table1.add(symbols.DataSymbol(
+        marker_name, symbols.UnsupportedFortranType(decl)))
+    table2.add(symbols.DataSymbol(
+        marker_name, symbols.UnsupportedFortranType(decl)))
+
+    old_sym = table2.lookup(marker_name)
+    table1._handle_symbol_clash(old_sym, table2)
+
+    assert len(table1.symbols) == 1
+    assert old_sym.name == marker_name
+
+
+def test_handle_symbol_clash_commonblock_overlap_with_other_marker():
+    '''Test that _handle_symbol_clash() scans all existing COMMON-block
+    markers and skips incoming marker when block names overlap.'''
+    table1 = symbols.SymbolTable()
+    table2 = symbols.SymbolTable()
+    marker_name = "_PSYCLONE_INTERNAL_COMMONBLOCK_1"
+    # Clash with same name but different block.
+    table1.add(symbols.DataSymbol(
+        marker_name, symbols.UnsupportedFortranType("common /other/ b")))
+    # Include a non-marker symbol so the marker filter condition also takes
+    # the false branch while scanning existing symbols.
+    table1.add(symbols.DataSymbol("plain", symbols.INTEGER_TYPE))
+    # Existing marker with different internal number but same block as incoming
+    # marker. This exercises the scan of all existing markers in table1.
+    table1.add(symbols.DataSymbol(
+        "_PSYCLONE_INTERNAL_COMMONBLOCK_2",
+        symbols.UnsupportedFortranType("common /overlap/ c")))
+    table2.add(symbols.DataSymbol(
+        marker_name, symbols.UnsupportedFortranType("common /overlap/ a")))
+
+    old_sym = table2.lookup(marker_name)
+    table1._handle_symbol_clash(old_sym, table2)
+
+    assert len(table1.symbols) == 3
+    assert old_sym.name == marker_name
+
+
+def test_handle_symbol_clash_commonblock_distinct_blocks_renamed():
+    '''Test that _handle_symbol_clash() renames and adds an incoming
+    COMMON-block marker when block names do not overlap.'''
+    table1 = symbols.SymbolTable()
+    table2 = symbols.SymbolTable()
+    marker_name = "_PSYCLONE_INTERNAL_COMMONBLOCK_1"
+    table1.add(symbols.DataSymbol(
+        marker_name, symbols.UnsupportedFortranType("common /first/ a")))
+    table2.add(symbols.DataSymbol(
+        marker_name, symbols.UnsupportedFortranType("common /second/ b")))
+
+    old_sym = table2.lookup(marker_name)
+    table1._handle_symbol_clash(old_sym, table2)
+
+    assert old_sym.name != marker_name
+    assert any(sym.datatype.declaration == "common /second/ b"
+               for sym in table1.symbols)
+
+
+def test_handle_symbol_clash_unsupported_fortran_non_commonblock_name():
+    '''Test that a clash between UnsupportedFortranType symbols with names
+    unrelated to common-block markers takes the standard rename-and-add path.
+    '''
+    table1 = symbols.SymbolTable()
+    table2 = symbols.SymbolTable()
+    table1.add(symbols.DataSymbol(
+        "clash", symbols.UnsupportedFortranType("type(t1) :: clash")))
+    table2.add(symbols.DataSymbol(
+        "clash", symbols.UnsupportedFortranType("type(t2) :: clash")))
+
+    old_sym = table2.lookup("clash")
+    table1._handle_symbol_clash(old_sym, table2)
+
+    assert old_sym.name != "clash"
+    assert any(sym.datatype.declaration == "type(t2) :: clash"
+               for sym in table1.symbols)
+
+
 def test_swap_symbol_properties():
     ''' Test the symboltable swap_properties method '''
     # pylint: disable=too-many-statements