Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions src/psyclone/psyir/symbols/symbol_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -706,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
Expand Down Expand Up @@ -947,13 +953,42 @@ def _add_symbols_from_table(self, other_table, symbols_to_skip=()):
already been updated to refer to a Container in this table.

'''
# pylint: disable=import-outside-toplevel
from psyclone.psyir.symbols.datatypes import UnsupportedFortranType

# Names (normalised) of commonblock variables already in this table.
Comment thread
arporter marked this conversation as resolved.
Outdated
# Used below to decide whether a _PSYCLONE_INTERNAL_COMMONBLOCK marker
# from other_table is a duplicate that must be skipped.
self_cb_names = frozenset(
self._normalize(s.name)
for s in self.symbols if s.is_commonblock)

# Normalised names of commonblock variables in other_table. Any
Comment thread
arporter marked this conversation as resolved.
Outdated
# overlap with self_cb_names means the COMMON block is already
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth extending this comment to say that the same variable cannot appear in more than one common block so if they have the same name and a 'common block' interface then they must originate from the same common. I'm slightly uneasy as the actual names of symbols within a common block don't have to match: just their order and type:

Image

So, two symbols from different scopes but with the same name and a common block interface are not guaranteed to be referring to the same thing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider:

integer :: var1, var2
common /block1/ var1, var2

and (in a different scope):

real :: var1, var2
common /block2/ var1, var2

as written, the code would assume that it is safe to ignore this clash because both var1 and var2 have a 'common block' interface. Unfortunately we currently make no attempt to capture the name of a common block - the whole common /.... statement is just reproduced blindly.

# declared in this table and the incoming marker is a duplicate.
other_cb_names = frozenset(
self._normalize(s.name)
for s in other_table.symbols if s.is_commonblock)
cb_overlap = bool(self_cb_names & other_cb_names)

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

# Skip _PSYCLONE_INTERNAL_COMMONBLOCK marker symbols when any of
# the COMMON-block variables they represent are already present in
# this table. Adding such a marker would produce a duplicate
# COMMON declaration and a Fortran compile error like
# "Symbol 'x' is already in a COMMON block".
if (cb_overlap and
self._normalize(old_sym.name).startswith(
Comment thread
arporter marked this conversation as resolved.
Outdated
"_psyclone_internal_commonblock") and
isinstance(old_sym.datatype, UnsupportedFortranType)):
continue

try:
self.add(old_sym)

Expand Down Expand Up @@ -1007,6 +1042,33 @@ def _handle_symbol_clash(self, old_sym, other_table):
# refer to the same thing and we don't have to do anything.
return

if old_sym.is_commonblock and self_sym.is_commonblock:
Comment thread
arporter marked this conversation as resolved.
Outdated
return

from psyclone.psyir.symbols.datatypes import UnsupportedFortranType
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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't right - they are just the same unsupported type, not necessarily a common block.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, this is quite confusing. I think what we have here is that the two symbols that have a name collision both have a 'common block' interface and also happen to be of UnsupportedFortranType. I think (but could be wrong) that it would also be possible for them to be of a supported type. Unfortunately, we have no way of knowing whether they are both the (same) variable from the same common block.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I'm still confused here. The check at L1038 means that only one or none of the clashing symbols has a common-block interface. Therefore, old_sym and self_sym can't both be common-block markers and in fact both might not be? Am I misunderstanding something?

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"):
import re # pylint: disable=import-outside-toplevel
Comment thread
arporter marked this conversation as resolved.
Outdated
_blk_re = re.compile(r"/\s*(\w*)\s*/", re.IGNORECASE)
old_blocks = set(_blk_re.findall(
old_sym.datatype.declaration))
self_blocks = set(_blk_re.findall(
self_sym.datatype.declaration))
if old_blocks & self_blocks:
Comment thread
arporter marked this conversation as resolved.
Outdated
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.
Expand Down
46 changes: 46 additions & 0 deletions src/psyclone/tests/psyir/transformations/inline_trans_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2844,3 +2844,49 @@ def test_apply_array_access_check_unresolved_override_option(
inline_trans.apply(
call, use_first_callee_and_no_arg_check=True)
# TODO check results


def test_apply_common_block_no_duplicate(fortran_reader, fortran_writer):
'''Test that inlining two routines that share a COMMON block does not
produce duplicate COMMON declarations (which would cause a Fortran compile
error "Symbol X is already in a COMMON block").'''

src_caller = """\
subroutine caller()
implicit none
call sub1()
call sub2()
end subroutine caller
"""
src_sub1 = """\
subroutine sub1()
implicit none
real :: volume, lmmpi
COMMON /blk/ volume, lmmpi
volume = 1.0
end subroutine sub1
"""
src_sub2 = """\
subroutine sub2()
implicit none
real :: volume, lmmpi
COMMON /blk/ volume, lmmpi
lmmpi = 2.0
end subroutine sub2
"""
caller = fortran_reader.psyir_from_source(src_caller).walk(Routine)[0]
sub1 = fortran_reader.psyir_from_source(src_sub1).walk(Routine)[0]
sub2 = fortran_reader.psyir_from_source(src_sub2).walk(Routine)[0]

trans = InlineTrans()
calls = caller.walk(Call)
trans.apply(calls[0], routine=sub1)
calls = caller.walk(Call)
trans.apply(calls[0], routine=sub2)

result = fortran_writer(caller)
# Exactly one COMMON declaration must appear.
assert result.count("COMMON /blk/") == 1
# Both variables must still be present.
assert "volume" in result
assert "lmmpi" in result
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a compilation check if possible.

Loading