Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Towards #2271, closes #2278, closes #2849) Generalise reference_accesses() and use to tidy KernelModuleInlineTrans. #2825

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
4435fa8
#2271 add reference_accesses() to Literal, CodeBlock and ScopingNode
arporter Dec 11, 2024
ec4045f
#2271 improve ScopingNode implementation
arporter Dec 11, 2024
76026ac
#2271 remove all Kern.local_vars() methods and tests
arporter Dec 11, 2024
b765e55
#2271 fix issue with CodeBlock and add test
arporter Dec 11, 2024
0920bf9
#2271 WIP adding ScopingNode tests [skip ci]
arporter Dec 11, 2024
8501cba
#2271 add support and tests for symbols within ArrayTypes [skip ci]
arporter Dec 12, 2024
d1127e8
#2271 fix all tests
arporter Dec 12, 2024
0b1d398
#2271 fix linting
arporter Dec 12, 2024
4651016
#2271 update docstring
arporter Dec 13, 2024
d8e639b
#2271 add new AccessType.CALL and use to distinguish symbols that are…
arporter Dec 17, 2024
f031e65
#2271 fix flake8
arporter Dec 17, 2024
300c563
#2271 rm include_calls argument
arporter Dec 17, 2024
1846fa6
Merge branch 'master' into 2271_ref_accesses
arporter Dec 17, 2024
0d4846c
#2825 add COMPILE_TIME and INQUIRY AccessTypes. Remove special option…
arporter Dec 19, 2024
26eabf5
#2271 fix linting
arporter Dec 19, 2024
f417bc1
Merge branch 'master' into 2271_ref_accesses
arporter Dec 20, 2024
5a7671e
#2271 work through implications of adding various inquiry access type…
arporter Dec 20, 2024
7991f62
#2271 fix remaining test failures
arporter Dec 20, 2024
c55c45e
#2271 fixes for linting
arporter Dec 20, 2024
b568451
#2271 fix implementation and test of change_read_to_write()
arporter Dec 20, 2024
e541be5
Merge branch 'master' into 2271_ref_accesses
arporter Jan 6, 2025
bf242e6
#2271 improvements for review
arporter Jan 6, 2025
278b82e
#2271 fix linting
arporter Jan 6, 2025
b646667
#2271 final changes for review
arporter Jan 6, 2025
1ed438e
#2271 fix linting
arporter Jan 6, 2025
3bab6b7
#2271 fix type hint for older python
arporter Jan 6, 2025
e389463
#2271 address coverage
arporter Jan 7, 2025
1b583ff
Merge branch 'master' into 2271_ref_accesses
arporter Jan 7, 2025
8541f08
Merge branch 'master' into 2271_ref_accesses
arporter Jan 8, 2025
ce58e0c
#2271 extend ScopingNode.reference_accesses to handle UnsupportedFort…
arporter Jan 8, 2025
c5ee43f
Merge branch 'master' into 2271_ref_accesses
arporter Jan 14, 2025
125db1a
#2271 add new hoist_trans test from #2849
arporter Jan 15, 2025
ade8c70
#2271 bring in SymbolTable fix from 2845 branch
arporter Jan 17, 2025
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
27 changes: 15 additions & 12 deletions src/psyclone/core/access_type.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@

'''This module implements the AccessType used throughout PSyclone.'''

from __future__ import print_function, absolute_import
from enum import Enum
from psyclone.configuration import Config

Expand All @@ -55,36 +54,40 @@ class AccessType(Enum):
# This is used internally to indicate unknown access type of
# a variable, e.g. when a variable is passed to a subroutine
# and the access type of this variable in the subroutine
# is unknown
# is unknown.
UNKNOWN = 7
# A symbol representing a routine is called.
CALL = 8

def __str__(self):
'''Convert to a string representation, returning just the
enum (e.g. 'WRITE')..
:return: API name for this string.
:rtype: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know in the past we didn't do it for __str__ but maybe now we can use the -> str typehint for the sake of helping IDEs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

enum (e.g. 'WRITE').
'''
# pylint complains without str() that the return type is not a str
return str(self.name)

def api_specific_name(self):
def api_specific_name(self) -> str:
'''This convenience function returns the name of the type in the
current API. E.g. in a lfric API, WRITE --> "gh_write"
current API. E.g. in the lfric API, WRITE --> "gh_write". If no
mapping is available then the generic name is returned.

:returns: The API specific name.
:rtype: str
'''
api_config = Config.get().api_conf()
rev_access_mapping = api_config.get_reverse_access_mapping()
return rev_access_mapping[self]
return rev_access_mapping.get(self, str(self).lower())

@staticmethod
def from_string(access_string):
def from_string(access_string: str):
'''Convert a string (e.g. "read") into the corresponding
AccessType enum value (AccessType.READ).

:param str access_string: Access type as string.
:param access_string: Access type as a string.

:returns: Corresponding AccessType enum.
:Raises: ValueError if access_string is not a valid access type.
:rtype: :py:class:`psyclone.core.access_type.AccessType`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just -> AccessType if this is a valid typehint?

Copy link
Member Author

Choose a reason for hiding this comment

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

This needs the SELF syntax you mentioned because we're inside the AccessType class here.


:raises: ValueError if access_string is not a valid access type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second semicolon is in the wrong place

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops.

'''
for access in AccessType:
if access.name == access_string.upper():
Expand Down
20 changes: 14 additions & 6 deletions src/psyclone/core/single_variable_access_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,14 @@ def var_name(self):
'''
return str(self._signature)

def is_called(self) -> bool:
'''
:returns: whether or not any accesses of this variable
represent a call.
'''
return any(info.access_type == AccessType.CALL for
info in self._accesses)

def is_written(self):
''':returns: True if this variable is written (at least once).
:rtype: bool
Expand All @@ -226,21 +234,21 @@ def is_written_first(self):
return len(self._accesses) > 0 and \
(self._accesses[0].access_type == AccessType.WRITE)

def is_read_only(self):
def is_read_only(self) -> bool:
'''Checks if this variable is always read, and never
written.

:returns: True if this variable is read only.
:rtype: bool
'''
return all(access_info.access_type == AccessType.READ
for access_info in self._accesses)

def is_read(self):
''':returns: True if this variable is read (at least once).
:rtype: bool
def is_read(self) -> bool:
'''
:returns: True if this variable is read (at least once).
'''
return any(access_info.access_type in AccessType.all_read_accesses()
read_accesses = AccessType.all_read_accesses()
return any(access_info.access_type in read_accesses
for access_info in self._accesses)

def has_read_write(self):
Expand Down
13 changes: 10 additions & 3 deletions src/psyclone/core/variables_access_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,14 @@ def merge(self, other_access_info):
# locations just merged in
self._location = self._location + max_new_location

def is_called(self, signature: Signature) -> bool:
'''
:param signature: signature of the variable.

:returns: True if the specified variable is called at least once.
'''
return self[signature].is_called()

def is_written(self, signature):
'''Checks if the specified variable signature is at least
written once.
Expand All @@ -336,15 +344,14 @@ def is_written(self, signature):
var_access_info = self[signature]
return var_access_info.is_written()

def is_read(self, signature):
def is_read(self, signature) -> bool:
'''Checks if the specified variable signature is at least read once.

:param signature: signature of the variable
:type signature: :py:class:`psyclone.core.Signature`

:returns: True if the specified variable name is read (at least \
:returns: True if the specified variable name is read (at least
once).
:rtype: bool

:raises: KeyError if the signature cannot be found.'''

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@

'''


from psyclone.core import VariablesAccessInfo
from psyclone.errors import InternalError
from psyclone.psyGen import Transformation, CodedKern
from psyclone.psyir.transformations import TransformationError
from psyclone.psyir.symbols import (
ContainerSymbol, DataSymbol, DataTypeSymbol, DefaultModuleInterface,
IntrinsicSymbol, RoutineSymbol, Symbol)
RoutineSymbol, Symbol)
from psyclone.psyir.nodes import (
Container, Reference, Routine, ScopingNode,
Literal, CodeBlock, Call, IntrinsicCall)
Expand Down Expand Up @@ -156,41 +156,20 @@ def validate(self, node, options=None):
# We do not support kernels that use symbols representing data
# declared in their own parent module (we would need to new imports
# from this module for those, and we don't do this yet).
Copy link
Collaborator

Choose a reason for hiding this comment

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

It was already here but I don't understand the parenthesis, did you mean "we would need to import them from this module, and we don't do this yet"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it wasn't quite right. I've updated it so that it hopefully makes sense.

# These can only be found in References and CodeBlocks.
for var in kernel_schedule.walk(Reference):
symbol = var.symbol
if isinstance(symbol, IntrinsicSymbol):
continue
if not symbol.is_import:
if not var.scope.symbol_table.lookup(
symbol.name, scope_limit=kernel_schedule,
otherwise=False):
raise TransformationError(
f"{kern_or_call} '{kname}' contains accesses to "
f"'{symbol.name}' which is declared in the same "
f"module scope. Cannot inline such a {kern_or_call}.")
for block in kernel_schedule.walk(CodeBlock):
for name in block.get_symbol_names():
# Is this quantity declared within the kernel?
sym = block.scope.symbol_table.lookup(
name, scope_limit=kernel_schedule, otherwise=None)
if not sym:
# It isn't declared in the kernel.
# Can we find the corresponding symbol at all?
sym = block.scope.symbol_table.lookup(name, otherwise=None)
if not sym:
raise TransformationError(
f"{kern_or_call} '{kname}' contains accesses to "
f"'{name}' in a CodeBlock but the origin of this "
f"symbol is unknown.")
# We found it in an outer scope - is it from an import or a
# declaration?
if not sym.is_import:
raise TransformationError(
f"{kern_or_call} '{kname}' contains accesses to "
f"'{name}' in a CodeBlock that is declared in the "
f"same module scope. Cannot inline such a "
f"{kern_or_call}.")
vai = VariablesAccessInfo(kernel_schedule)
table = kernel_schedule.symbol_table
for sig in vai.all_signatures:
symbol = table.lookup(sig.var_name, otherwise=None)
Comment on lines +163 to +166
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this new version, but note that this comes with the signatures limitations e.g. don't work with nested scopes:

  • if the variable has a unique name
subroutine test()
    do i = 1, 10 <- PSyIR declares "a" here
        a(i) = 1
    end do 
end subroutine

and will fails with the TransformationError below.

  • if the variable has a name clash
integer :: a
subroutine test()
    do i = 1, 10 <- PSyIR declares "a" here
        a(i) = 1
    end do 
end subroutine

it will fails with the "is declared in the same module scope".

Both cases are wrong and the subroutine could be inlined. Granted that this won't happen often because it requires inlining subroutines that have previously been transformed with psyclone so that a symbol was declared in a inner scope. But maybe we should document - add an xfail to #2424

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't get the first case to fail but I've added an x-failing test for the second one and added a TODO and text to a comment in the code too.

if not symbol:
raise TransformationError(
f"{kern_or_call} '{kname}' contains accesses to "
f"'{sig.var_name}' but the origin of this symbol is "
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/symbol/signature/ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.

f"unknown.")
if not symbol.is_import and symbol.name not in table:
raise TransformationError(
f"{kern_or_call} '{kname}' contains accesses to "
f"'{symbol.name}' which is declared in the same "
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/same/callee/ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's better, thanks.

f"module scope. Cannot inline such a {kern_or_call}.")

# We can't transform subroutines that shadow top-level symbol module
# names, because we won't be able to bring this into the subroutine
Expand Down
14 changes: 14 additions & 0 deletions src/psyclone/domain/lfric/lfric_extract_driver_creator.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,11 @@ def _sym_is_field(sym):
read_var = f"{psy_data.name}%ReadVariable"
mod_man = ModuleManager.get()

# Construct a set of the names of all LFRic kind parameters.
const = LFRicConstants()
all_kind_names = set(
[val["kind"] for val in const.DATA_TYPE_MAP.values()])

# First handle variables that are read:
# -------------------------------------
for module_name, signature in read_write_info.read_list:
Expand All @@ -583,6 +588,11 @@ def _sym_is_field(sym):
# variables have References, and will already have been declared
# in the symbol table (in _add_all_kernel_symbols).
sig_str = self._flatten_signature(signature)

# Work-around for the fact that we want to exclude kind parameters.
if sig_str in all_kind_names:
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this is a symptom that this (and the DataTypeSymbol check added below) are misclassified as READ (and therefore the driver was to make a copy) but they should be TYPE_INFO (and therefore the driver shouldn't consider them for the value copy)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this has gone now.


if module_name:
mod_info = mod_man.get_module_info(module_name)
orig_sym = mod_info.get_symbol(signature[0])
Expand All @@ -593,6 +603,10 @@ def _sym_is_field(sym):
else:
orig_sym = original_symbol_table.lookup(signature[0])

if orig_sym and isinstance(orig_sym, DataTypeSymbol):
# We don't want symbols representing data types.
continue

if orig_sym and orig_sym.is_array and _sym_is_field(orig_sym):
# This is a field vector, so add all individual fields
upper = int(orig_sym.datatype.shape[0].upper.value)
Expand Down
7 changes: 0 additions & 7 deletions src/psyclone/gocean1p0.py
Original file line number Diff line number Diff line change
Expand Up @@ -1221,13 +1221,6 @@ def reference_accesses(self, var_accesses):
super().reference_accesses(var_accesses)
var_accesses.next_location()

def local_vars(self):
'''Return a list of the variable (names) that are local to this loop
(and must therefore be e.g. threadprivate if doing OpenMP)

'''
return []

@property
def index_offset(self):
''' The grid index-offset convention that this kernel expects '''
Expand Down
17 changes: 0 additions & 17 deletions src/psyclone/psyGen.py
Original file line number Diff line number Diff line change
Expand Up @@ -1323,9 +1323,6 @@ def is_coloured(self):
def iterates_over(self):
return self._iterates_over

def local_vars(self):
raise NotImplementedError("Kern.local_vars should be implemented")

def gen_code(self, parent):
raise NotImplementedError("Kern.gen_code should be implemented")

Expand Down Expand Up @@ -1815,14 +1812,6 @@ def _validate_child(position, child):
'''
return position == 0 and isinstance(child, Schedule)

@abc.abstractmethod
def local_vars(self):
'''
:returns: list of the variable (names) that are local to this kernel \
(and must therefore be e.g. threadprivate if doing OpenMP)
:rtype: list of str
'''

def node_str(self, colour=True):
''' Returns the name of this node with (optional) control codes
to generate coloured output in a terminal that supports it.
Expand Down Expand Up @@ -1867,12 +1856,6 @@ def load(self, call, arguments, parent=None):
name = call.ktype.name
super(BuiltIn, self).__init__(parent, call, name, arguments)

def local_vars(self):
'''Variables that are local to this built-in and therefore need to be
made private when parallelising using OpenMP or similar. By default
builtin's do not have any local variables so set to nothing'''
return []


class Arguments():
'''
Expand Down
19 changes: 12 additions & 7 deletions src/psyclone/psyir/nodes/call.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,10 +315,14 @@ def reference_accesses(self, var_accesses):
# We conservatively default to READWRITE otherwise (TODO #446).
default_access = AccessType.READWRITE

# TODO #2271: This may skip references in inner expressions of
# structure calls, but to implement properly we new a new kind of
# AccessType that represents being called (USED but not READ, maybe
# the same that we need for INQUIRY type attributes?)
# The RoutineSymbol has a CALL access.
sig, indices_list = self.routine.get_signature_and_indices()
var_accesses.add_access(sig, AccessType.CALL, self.routine)
# Any symbols referenced in any index expressions are READ.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe # Continue processing references in any index expression

As they are not all reads, e.g. there may be another CALL, even with and output argument which would be WRITE

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same in a similar comment below which was already here

Copy link
Member Author

Choose a reason for hiding this comment

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

Done and done.

for indices in indices_list:
for idx in indices:
idx.reference_accesses(var_accesses)

for arg in self.arguments:
if isinstance(arg, Reference):
# This argument is pass-by-reference.
Expand Down Expand Up @@ -373,12 +377,13 @@ def is_elemental(self):
@property
def is_pure(self):
'''
:returns: whether the routine being called is pure (guaranteed to \
return the same result when provided with the same argument \
:returns: whether the routine being called is pure (guaranteed to
return the same result when provided with the same argument
values). If this information is not known then it returns None.
:rtype: NoneType | bool
'''
if self.routine and self.routine.symbol:
if (self.routine and self.routine.symbol and
isinstance(self.routine.symbol, RoutineSymbol)):
return self.routine.symbol.is_pure
return None

Expand Down
23 changes: 22 additions & 1 deletion src/psyclone/psyir/nodes/codeblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
from enum import Enum
from fparser.two import Fortran2003
from fparser.two.utils import walk
from psyclone.core import AccessType, Signature, VariablesAccessInfo
from psyclone.psyir.nodes.statement import Statement
from psyclone.psyir.nodes.datanode import DataNode

Expand Down Expand Up @@ -174,8 +175,28 @@ def get_symbol_names(self):
Fortran2003.End_If_Stmt)):
# We don't want labels associated with loop or branch control.
result.append(node.string)

# Precision on literals requires special attention since they are just
# stored in the tree as str (fparser/#456).
for node in walk(parse_tree, (Fortran2003.Int_Literal_Constant,
Fortran2003.Real_Literal_Constant)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also do bools and chars ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, my mistake. For bonus points I've done complex too :-) The only other type is 'boz' and they don't have a kind parameter.

if node.items[1]:
result.append(node.items[1])
return result

def reference_accesses(self, var_accesses: VariablesAccessInfo):
'''
Get all variable access information. Since this is a CodeBlock we
only know the names of symbols accessed within it but not how they
are accessed. Therefore we err on the side of caution and mark
them all as READWRITE.

:param var_accesses: VariablesAccessInfo instance that stores the
information about variable accesses.

'''
for name in self.get_symbol_names():
var_accesses.add_access(Signature(name), AccessType.READWRITE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that we use READWRITE as worst case scenario, but shouldn't we use UNKNOWN here? It fits better for Codeblocks but it may require changes in the VAI helper methods which I am not sure they consider UNKNOWN, so maybe is for another PR if it needs more thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it would fit better but, as you suspect, the VAI and associated helper methods never mention UNKNOWN so we'd need to take care there.

self)

def __str__(self):
return f"CodeBlock[{len(self._fp2_nodes)} nodes]"
4 changes: 4 additions & 0 deletions src/psyclone/psyir/nodes/directive.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ def create_data_movement_deep_copy_refs(self):
node = vinfo.all_accesses[0].node
sym = table.lookup(sig.var_name)

if var_info.is_called(sig):
# Ignore accesses that are calls.
continue

if isinstance(sym.datatype, ScalarType):
# We ignore scalars as these are typically copied by value.
continue
Expand Down
Loading
Loading