-
Notifications
You must be signed in to change notification settings - Fork 33
Array index analysis using SMT #3213
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
base: master
Are you sure you want to change the base?
Changes from 22 commits
e3d40fd
ebd03ab
f46740f
6385bd2
7567511
c04c914
2488ed0
06c7b9a
9f62114
cde3a8f
6e5c443
f6c42f5
7248372
9eef149
9116168
558c5f9
5cb7280
952c0d2
3a1dcef
c0c3b78
e9a7508
f7079aa
b9154cd
df713f2
80c3428
08a1d93
e7e3413
b228cb0
80e831e
f69fa4f
8fdde19
3d99d60
cb0886b
7e17ed2
dc0bba0
a14b432
21f7842
88209ec
5a043bf
da86c52
d21eae5
1bb4705
b8989d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you sort them alphabetically, this helps with avoiding duplicates :) |
||
| ArrayIndexAnalysisOptions) | ||
|
|
||
| # For AutoAPI documentation generation. | ||
| __all__ = ['CallTreeUtils', | ||
| 'DTCode', | ||
| 'DependencyTools', | ||
| 'DefinitionUseChain', | ||
| 'ReadWriteInfo', | ||
| 'ReductionInferenceTool'] | ||
| 'ReductionInferenceTool', | ||
| 'ArrayIndexAnalysis', | ||
| 'ArrayIndexAnalysisOptions'] | ||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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[ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have switched to use 'proper' Python typing (instead of and remove the existing
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done (commit df713f2) but only for the the method in question, not throughout the file. |
||
| bool, ArrayIndexAnalysisOptions] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to admit I don't like this kind of Could we instead just use I think my main issue is that it does not 'read' nice: the variable is
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done (commit df713f2). |
||
|
|
||
| :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() | ||
|
|
@@ -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): | ||
|
|
@@ -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: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had the same thought myself. I might return to this at some point, as I think a bit more work would be needed to justify it right now. One advantage of keeping them separate was that it allowed me to find some issues with the existing DA (which I think you have now fixed).
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you a ticket for further Z3 work and add a #TODO here. |
||
| # This analysis runs after the loop | ||
| par_able = True | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be a bit easier to understand if |
||
| 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) | ||
|
|
@@ -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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The analysis now generates conflict messages (commit 80c3428). As an example, the loop yields the following message for the These messages get converted to DA |
||
| "array accesses in the loop may be conflicting " | ||
| "and hence cannot be parallelised.", | ||
| DTCode.ERROR_DEPENDENCY) | ||
| result = False | ||
|
|
||
| return result | ||
|
|
||
| # ------------------------------------------------------------------------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 \ | ||
|
|
@@ -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) | ||
|
|
@@ -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( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that seems cleaner. Done in commit 08a1d93. |
||
| "use_smt_array_index_analysis", False) | ||
|
|
||
| # Check type of reduction_ops (not handled by validate_options) | ||
| if not isinstance(reduction_ops, list): | ||
|
|
@@ -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] | ||
|
|
||
|
|
@@ -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 | ||
|
|
@@ -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: | ||
|
|
@@ -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. | ||
|
|
@@ -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 | ||
|
|
||
There was a problem hiding this comment.
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:(if not, we need a test for that, the code passes the existing tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (commit b9154cd).