Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
3 changes: 2 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ def get_files(directory, install_path, valid_suffixes):
packages=PACKAGES,
package_dir={"": "src"},
install_requires=['pyparsing', 'fparser>=0.2.1', 'configparser',
'sympy', "Jinja2", 'termcolor', 'graphviz'],
'sympy', "Jinja2", 'termcolor', 'graphviz',
'z3-solver'],
extras_require={
'doc': ["sphinx", "sphinxcontrib.bibtex", "sphinx_design",
"pydata-sphinx-theme", "sphinx-autodoc-typehints",
Expand Down
20 changes: 20 additions & 0 deletions src/psyclone/psyir/nodes/if_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
# Authors R. W. Ford, A. R. Porter, S. Siso and N. Nobre, STFC Daresbury Lab
# I. Kavcic, Met Office
# J. Henrichs, Bureau of Meteorology
# M. Naylor, University of Cambridge
# -----------------------------------------------------------------------------

''' This module contains the IfBlock node implementation.'''
Expand Down Expand Up @@ -195,3 +196,22 @@ def reference_accesses(self) -> VariablesAccessMap:
if self.else_body:
var_accesses.update(self.else_body.reference_accesses())
return var_accesses

def flat(self):
'''This method allows a chain of 'if'/'else if'/.../'else'
statements to be viewed in its flattened form, without nesting.

:returns: a list of condition/body pairs. Nested 'else if' chains
(if there are any) are recursively gathered. The condition for
the final 'else' in the chain (if there is one) is 'None'.
'''
if self.else_body is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this harder to understand than necessary (probably because adding 'this' block's data at the end with an insert). As far as I can tell, the following code produces the same result:

        branches = [(self.condition, self.if_body)]
        if self.else_body:
            if (isinstance(self.else_body, Schedule) and
                    len(self.else_body.children) == 1 and
                    isinstance(self.else_body.children[0], IfBlock)):
                branches.extend(self.else_body.children[0].flat())
            else:
                branches.append((None, self.else_body))
        return branches

(if not, we need a test for that, the code passes the existing tests).

branches = []
elif (isinstance(self.else_body, Schedule) and
len(self.else_body.children) == 1 and
isinstance(self.else_body.children[0], IfBlock)):
branches = self.else_body.children[0].flat()
else:
branches = [(None, self.else_body)]
branches.insert(0, (self.condition, self.if_body))
return branches
6 changes: 5 additions & 1 deletion src/psyclone/psyir/tools/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,15 @@
from psyclone.psyir.tools.read_write_info import ReadWriteInfo
from psyclone.psyir.tools.definition_use_chains import DefinitionUseChain
from psyclone.psyir.tools.reduction_inference import ReductionInferenceTool
from psyclone.psyir.tools.array_index_analysis import (ArrayIndexAnalysis,
ArrayIndexAnalysisOptions)

# For AutoAPI documentation generation.
__all__ = ['CallTreeUtils',
'DTCode',
'DependencyTools',
'DefinitionUseChain',
'ReadWriteInfo',
'ReductionInferenceTool']
'ReductionInferenceTool',
'ArrayIndexAnalysis',
'ArrayIndexAnalysisOptions']
1,099 changes: 1,099 additions & 0 deletions src/psyclone/psyir/tools/array_index_analysis.py

Large diffs are not rendered by default.

43 changes: 39 additions & 4 deletions src/psyclone/psyir/tools/dependency_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
from psyclone.psyir.backend.sympy_writer import SymPyWriter
from psyclone.psyir.backend.visitor import VisitorError
from psyclone.psyir.nodes import Loop, Node, Range
from psyclone.psyir.tools.array_index_analysis import (
ArrayIndexAnalysis, ArrayIndexAnalysisOptions)


# pylint: disable=too-many-lines
Expand Down Expand Up @@ -162,11 +164,20 @@ class DependencyTools():
specified in the PSyclone config file. This can be used to
exclude for example 1-dimensional loops.
:type loop_types_to_parallelise: Optional[List[str]]
:param use_smt_array_index_analysis: if True, the SMT-based
array index analysis will be used for detecting array access
conflicts. An ArrayIndexAnalysisOptions value can also be given,
instead of a bool, in which case the analysis will be invoked
with the given options.
:type use_smt_array_index_analysis: Union[
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have switched to use 'proper' Python typing (instead of :type:), and are fixing files as we touch them. Could you update this to use:

                 loop_types_to_parallelise: Bool = None,
                 use_smt_array_index_analysis: Union[...] = False):

and remove the existing :type: comments? Thanks

bool, ArrayIndexAnalysisOptions]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have to admit I don't like this kind of Union here. Yes, it actually works fine, but I would guess that a linter will have problems with that (and yes, atm we don't do linting for typing, but that might come at some stage :) ).

Could we instead just use use_smt_array_index_analysis: Optional[...] = None? Then add a enabled option to the ArrayIndexAnalysisOptions class. So, if no ArrayIndexAnalysisOptions is specified, you can set self._smt_array_analysis_option = ArrayIndexAnalysisOptions() and set the enabled option to false. This way, the dependency analysis member will have a single type only, removing the need for isinstance tests later.

I think my main issue is that it does not 'read' nice: the variable is use_..., but it's sometimes not a boolean.


:raises TypeError: if an invalid loop type is specified.

'''
def __init__(self, loop_types_to_parallelise=None):
def __init__(self,
loop_types_to_parallelise=None,
use_smt_array_index_analysis=False):
if loop_types_to_parallelise:
# Verify that all loop types specified are valid:
config = Config.get()
Expand All @@ -183,6 +194,7 @@ def __init__(self, loop_types_to_parallelise=None):
else:
self._loop_types_to_parallelise = []
self._clear_messages()
self._use_smt_array_index_analysis = use_smt_array_index_analysis

# -------------------------------------------------------------------------
def _clear_messages(self):
Expand Down Expand Up @@ -884,9 +896,15 @@ def can_loop_be_parallelised(self, loop,
# TODO #1270 - the is_array_access function might be moved
is_array = symbol.is_array_access(access_info=var_info)
if is_array:
# Handle arrays
par_able = self._array_access_parallelisable(loop_vars,
var_info)
# If using the SMT-based array index analysis then do
# nothing for now. This analysis is run after the loop.
if self._use_smt_array_index_analysis:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to first run the existing array analysis? And only run the the Z3 solver if the result is false (and long term, maybe the existing analysis could return three values: yes, no, unknown.

I might try to do some additional performance tests (I noticed the results you have posted ;) ), this obviously only makes sense if the existing one is significantly faster.

# This analysis runs after the loop
par_able = True
else:
# Handle arrays
par_able = self._array_access_parallelisable(loop_vars,
var_info)
else:
# Handle scalar variable
par_able = self._is_scalar_parallelisable(signature, var_info)
Expand All @@ -898,6 +916,23 @@ def can_loop_be_parallelised(self, loop,
# not just the first one
result = False

# Apply the SMT-based array index analysis, if enabled
if self._use_smt_array_index_analysis:
if isinstance(self._use_smt_array_index_analysis,
ArrayIndexAnalysisOptions):
options = self._use_smt_array_index_analysis
else:
options = ArrayIndexAnalysisOptions()
analysis = ArrayIndexAnalysis(options)
conflict_free = analysis.is_loop_conflict_free(loop)
if not conflict_free:
self._add_message(
"The ArrayIndexAnalysis has determined that the"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get additional information (esp. which variable is the problem)? Ideally, these messages should be added in the new class. One way of doing this would be to pass in the DependencyAnalysis instance to is_loop_conflict_free (so it could use the add_message method). Or we could even split the message handling into a stand-alone class.
I am happy for now to just open a ticket and add a TODO here.

"array accesses in the loop may be conflicting "
"and hence cannot be parallelised.",
DTCode.ERROR_DEPENDENCY)
result = False

return result

# -------------------------------------------------------------------------
Expand Down
31 changes: 26 additions & 5 deletions src/psyclone/psyir/transformations/parallel_loop_trans.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@
BinaryOperation, IntrinsicCall
)
from psyclone.psyir.tools import (
DependencyTools, DTCode, ReductionInferenceTool
DependencyTools, DTCode, ReductionInferenceTool,
ArrayIndexAnalysisOptions
)
from psyclone.psyir.transformations.loop_trans import LoopTrans
from psyclone.psyir.transformations.async_trans_mixin import \
Expand Down Expand Up @@ -175,6 +176,8 @@ def validate(self, node, options=None, **kwargs):
reduction_ops = self.get_option("reduction_ops", **kwargs)
if reduction_ops is None:
reduction_ops = []
use_smt_array_index_analysis = self.get_option(
"use_smt_array_index_analysis", **kwargs)
else:
verbose = options.get("verbose", False)
collapse = options.get("collapse", False)
Expand All @@ -185,6 +188,8 @@ def validate(self, node, options=None, **kwargs):
sequential = options.get("sequential", False)
privatise_arrays = options.get("privatise_arrays", False)
reduction_ops = options.get("reduction_ops", [])
use_smt_array_index_analysis = options.get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm - I see now how the handling of the dependency tools interacts with the transformation.
Looking at the transformation, there is already a place in apply where we create dependency tools, and pass it to independent_iterations.
What about we use the same pattern? I.e. change this transformation to accept an optional DependencyTool instance (if not, it creates a default with default setting for your analysys).
This would future-proof the design for any future extensions.

"use_smt_array_index_analysis", False)

# Check type of reduction_ops (not handled by validate_options)
if not isinstance(reduction_ops, list):
Expand Down Expand Up @@ -260,7 +265,8 @@ def validate(self, node, options=None, **kwargs):
f" object containing str representing the "
f"symbols to ignore, but got '{ignore_dependencies_for}'.")

dep_tools = DependencyTools()
dep_tools = DependencyTools(
use_smt_array_index_analysis=use_smt_array_index_analysis)

signatures = [Signature(name) for name in ignore_dependencies_for]

Expand Down Expand Up @@ -326,6 +332,8 @@ def apply(self, node, options=None, verbose: bool = False,
nowait: bool = False,
reduction_ops: List[Union[BinaryOperation.Operator,
IntrinsicCall.Intrinsic]] = None,
use_smt_array_index_analysis:
Union[bool, ArrayIndexAnalysisOptions] = False,
**kwargs):
'''
Apply the Loop transformation to the specified node in a
Expand Down Expand Up @@ -370,6 +378,11 @@ def apply(self, node, options=None, verbose: bool = False,
:param reduction_ops: if non-empty, attempt parallelisation
of loops by inferring reduction clauses involving any of
the reduction operators in the list.
:param use_smt_array_index_analysis: if True, the SMT-based
array index analysis will be used for detecting array access
conflicts. An ArrayIndexAnalysisOptions value can also be given,
instead of a bool, in which case the analysis will be invoked
with the given options.

'''
if not options:
Expand All @@ -378,7 +391,9 @@ def apply(self, node, options=None, verbose: bool = False,
ignore_dependencies_for=ignore_dependencies_for,
privatise_arrays=privatise_arrays,
sequential=sequential, nowait=nowait,
reduction_ops=reduction_ops, **kwargs
reduction_ops=reduction_ops,
use_smt_array_index_analysis=use_smt_array_index_analysis,
**kwargs
)
# Rename the input options that are renamed in this apply method.
# TODO 2668, rename options to be consistent.
Expand All @@ -399,16 +414,22 @@ def apply(self, node, options=None, verbose: bool = False,
privatise_arrays = options.get("privatise_arrays", False)
nowait = options.get("nowait", False)
reduction_ops = options.get("reduction_ops", [])
use_smt_array_index_analysis = options.get(
"use_smt_array_index_analysis", False)

self.validate(node, options=options, verbose=verbose,
collapse=collapse, force=force,
ignore_dependencies_for=ignore_dependencies_for,
privatise_arrays=privatise_arrays,
sequential=sequential, nowait=nowait,
reduction_ops=reduction_ops, **kwargs)
reduction_ops=reduction_ops,
use_smt_array_index_analysis=(
use_smt_array_index_analysis),
**kwargs)

list_of_signatures = [Signature(name) for name in list_of_names]
dtools = DependencyTools()
dtools = DependencyTools(
use_smt_array_index_analysis=use_smt_array_index_analysis)

# Add all reduction variables inferred by 'validate' to the list
# of signatures to ignore
Expand Down
70 changes: 70 additions & 0 deletions src/psyclone/tests/psyir/nodes/if_block_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
# Authors R. W. Ford, A. R. Porter, S. Siso and N. Nobre, STFC Daresbury Lab
# I. Kavcic, Met Office
# J. Henrichs, Bureau of Meteorology
# M. Naylor, University of Cambridge
# -----------------------------------------------------------------------------

''' Performs py.test tests on the IfBlock PSyIR node. '''
Expand Down Expand Up @@ -282,3 +283,72 @@ def test_ifblock_children_validation():
ifblock.addchild(else_body)
assert ("Item 'Schedule' can't be child 3 of 'If'. The valid format is: "
"'DataNode, Schedule [, Schedule]'." in str(excinfo.value))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add a test with no else branch? ATM that line in if_block.py is not covered. My suggested re-writing avoids this issue (since there is no code to execute if there is no else block :) ), but still we should have a separate test that covers the no-else case.


def test_if_block_flat_full(fortran_reader, fortran_writer):
'''Test the IfBlock's flat() method on a fully flattenable chain
'''
psyir = fortran_reader.psyir_from_source('''
subroutine sub(a, b, c, result)
logical, intent(in) :: a, b, c
integer, intent(out) :: result
if (a) then
result = 1
else if (b) then
result = 2
else if (c) then
result = 3
else
result = 4
end if
end subroutine''')
if_block = psyir.walk(IfBlock)[0]
branches = if_block.flat()
# The flat view of the IfBlock should give 4 branches
assert len(branches) == 4
# The condition of the final 'else' branch should be 'None'
assert branches[3][0] is None
# Check the other conditions
assert (fortran_writer(branches[0][0]) == "a")
assert (fortran_writer(branches[1][0]) == "b")
assert (fortran_writer(branches[2][0]) == "c")
# Check the bodies
assert (fortran_writer(branches[0][1]).startswith("result = 1"))
assert (fortran_writer(branches[1][1]).startswith("result = 2"))
assert (fortran_writer(branches[2][1]).startswith("result = 3"))
assert (fortran_writer(branches[3][1]).startswith("result = 4"))


def test_if_block_flat_partial(fortran_reader, fortran_writer):
'''Test the IfBlock's flat() method on a partially flattenable chain
'''
psyir = fortran_reader.psyir_from_source('''
subroutine sub(a, b, c, result)
logical, intent(in) :: a, b, c
integer, intent(out) :: result
if (a) then
result = 1
else if (b) then
result = 2
else
if (c) then
result = 3
else
result = 4
end if
result = result + 1
end if
end subroutine''')
if_block = psyir.walk(IfBlock)[0]
branches = if_block.flat()
# The flat view of the IfBlock should give 3 branches
assert len(branches) == 3
# The condition of the final 'else' branch should be 'None'
assert branches[2][0] is None
# Check the other conditions
assert (fortran_writer(branches[0][0]) == "a")
assert (fortran_writer(branches[1][0]) == "b")
# Check the bodies
assert (fortran_writer(branches[0][1]).startswith("result = 1"))
assert (fortran_writer(branches[1][1]).startswith("result = 2"))
assert (fortran_writer(branches[2][1]).startswith("if (c)"))
38 changes: 38 additions & 0 deletions src/psyclone/tests/psyir/nodes/omp_directives_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5349,3 +5349,41 @@ def test_firstprivate_with_uninitialised(fortran_reader, fortran_writer):
output = fortran_writer(psyir)
assert "firstprivate(a)" in output
assert "firstprivate(b)" in output


def test_array_analysis_option(fortran_reader, fortran_writer):
'''Test that a tiled loop can be parallelised when using the SMT-based
array index analysis.
'''
psyir = fortran_reader.psyir_from_source('''
subroutine my_matmul(a, b, c)
integer, dimension(:,:), intent(in) :: a
integer, dimension(:,:), intent(in) :: b
integer, dimension(:,:), intent(out) :: c
integer :: x, y, k, k_out_var, x_out_var, y_out_var, a1_n, a2_n, b1_n

a2_n = SIZE(a, 2)
b1_n = SIZE(b, 1)
a1_n = SIZE(a, 1)

c(:,:) = 0
do y_out_var = 1, a2_n, 8
do x_out_var = 1, b1_n, 8
do k_out_var = 1, a1_n, 8
do y = y_out_var, MIN(y_out_var + (8 - 1), a2_n), 1
do x = x_out_var, MIN(x_out_var + (8 - 1), b1_n), 1
do k = k_out_var, MIN(k_out_var + (8 - 1), a1_n), 1
c(x,y) = c(x,y) + a(k,y) * b(x,k)
enddo
enddo
enddo
enddo
enddo
enddo
end subroutine my_matmul''')
omplooptrans = OMPLoopTrans(omp_directive="paralleldo")
loop = psyir.walk(Loop)[0]
omplooptrans.apply(
loop, collapse=True, use_smt_array_index_analysis=True)
output = fortran_writer(psyir)
assert "collapse(2)" in output
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make it 'more' obvious that this implies that OpenMP has been applied? Either by adding a comment saying if omp was not applied, this would raise an exception, or by checking for the longer string !$omp parallel do collapse(2)

Loading
Loading