-
Notifications
You must be signed in to change notification settings - Fork 33
Changes to OpenMP scripts to extract arguments from iom_put #3373
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 29 commits
52b962a
dae4e86
34aeaac
a95ece0
d79fae1
46d6dfa
6339e55
6ce8a38
7c98fd4
dd3e62a
e06176b
ed69361
af9ea3d
a912e88
78127da
c3804eb
0420aa4
470ed98
cf4947e
c6c718c
03e0ffa
93d814c
f147e94
339f8b4
d9cced4
51f5e71
4fb5d7b
65fa992
19d77ef
fa35a58
78b4345
93c0b20
51c250a
5ad2f3e
18dae4e
b89289f
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 |
|---|---|---|
|
|
@@ -41,15 +41,15 @@ | |
| from psyclone.domain.common.transformations import KernelModuleInlineTrans | ||
| from psyclone.psyir.nodes import ( | ||
| Assignment, Loop, Directive, Node, Reference, CodeBlock, Call, | ||
| Routine, Schedule, IntrinsicCall, StructureReference, IfBlock) | ||
| from psyclone.psyir.symbols import DataSymbol | ||
| Routine, Schedule, IntrinsicCall, StructureReference, IfBlock, | ||
| Operation) | ||
| from psyclone.psyir.symbols import DataSymbol, ArrayType | ||
| from psyclone.psyir.transformations import ( | ||
| ArrayAssignment2LoopsTrans, HoistLoopBoundExprTrans, HoistLocalArraysTrans, | ||
| HoistTrans, InlineTrans, Maxval2LoopTrans, Sum2LoopTrans, Minval2LoopTrans, | ||
| Product2LoopTrans, ProfileTrans, OMPMinimiseSyncTrans, | ||
| Reference2ArrayRangeTrans, ScalarisationTrans, IncreaseRankLoopArraysTrans, | ||
| MaximalRegionTrans) | ||
| from psyclone.transformations import TransformationError | ||
| MaximalRegionTrans, TransformationError, DataNodeToTempTrans) | ||
|
|
||
| # USE statements to chase to gather additional symbol information. | ||
| NEMO_MODULES_TO_IMPORT = [ | ||
|
|
@@ -530,3 +530,22 @@ def _satisfies_minimum_region_rules(self, region: list[Node]) -> bool: | |
| routine_name = parent_routine.name if parent_routine else "" | ||
| if routine_name not in PROFILING_IGNORE: | ||
| MaximalProfilingOutsideDirectivesTrans().apply(children) | ||
|
|
||
|
|
||
| def iom_put_argument_to_temporary(calls: list[Call]): | ||
|
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. "param:" missing
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. Added. I also have slightly changed the behaviour so I will rerun integration. |
||
| '''Extracts the second argument of all iom_put calls and puts them | ||
| in a temporary if they are an Operation with an array datatype.''' | ||
| for call in calls: | ||
| if call.symbol.name == "iom_put": | ||
| for arg in call.arguments: | ||
| dtype = arg.datatype | ||
| if (isinstance(dtype, ArrayType) and | ||
| isinstance(arg, Operation)): | ||
| try: | ||
| DataNodeToTempTrans().apply(arg) | ||
| except TransformationError as err: | ||
| call.append_preceding_comment( | ||
|
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. Also, consider doing the same but inside the transformation with a verbose option.
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, should be covered by tests too.
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. Thanks, now you can delete this in favour of the option.
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. |
||
| f"Couldn't pull the argument {arg} to a " | ||
| f"temporary due to the following error: " | ||
| f"{str(err.value)}" | ||
| ) | ||
|
sergisiso marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -565,8 +565,11 @@ def _maxval_return_type(node: IntrinsicCall) -> DataType: | |
|
|
||
| :returns: the computed datatype for the IntrinsicCall. | ||
| """ | ||
| dtype = ScalarType( | ||
|
sergisiso marked this conversation as resolved.
|
||
| node.argument_by_name("array").datatype.intrinsic, | ||
| node.argument_by_name("array").datatype.precision | ||
| ) | ||
| arg = node.argument_by_name("array") | ||
| dtype = arg.datatype.elemental_type | ||
| if "dim" not in node.argument_names: | ||
| return dtype | ||
| # We have a dimension specified. We don't know the resultant shape | ||
|
|
@@ -588,8 +591,8 @@ def _dot_product_return_type(node: IntrinsicCall) -> DataType: | |
| from psyclone.psyir.tools.type_info_computation import ( | ||
| compute_scalar_type | ||
| ) | ||
| veca_datatype = node.argument_by_name("vector_a").datatype | ||
| vecb_datatype = node.argument_by_name("vector_b").datatype | ||
| veca_datatype = node.argument_by_name("vector_a").datatype.elemental_type | ||
|
sergisiso marked this conversation as resolved.
|
||
| vecb_datatype = node.argument_by_name("vector_b").datatype.elemental_type | ||
| return compute_scalar_type( | ||
| [ScalarType( | ||
| veca_datatype.intrinsic, veca_datatype.precision | ||
|
|
@@ -3215,7 +3218,9 @@ class Intrinsic(IAttr, Enum): | |
| optional_args={"kind": DataNode}, | ||
| return_type=lambda node: ( | ||
| _type_of_scalar_with_optional_kind( | ||
| node, node.argument_by_name("l").datatype.intrinsic, | ||
| node, | ||
| node.argument_by_name("l"). | ||
| datatype.intrinsic, | ||
|
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. Keep this in the same line.
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. 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. Wasn't done? There is also the same unecessary line breaks in lines 3775, 4458, 4730 and 4755
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. Ah my bad, I reverted some commits to the file I think and so this was also reverted.
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. Fixed those now. |
||
| "kind", | ||
| ) if "kind" in node.argument_names else | ||
| _type_of_named_argument(node, "l") | ||
|
|
@@ -3763,8 +3768,10 @@ class Intrinsic(IAttr, Enum): | |
| optional_args={"vector": DataNode}, | ||
| return_type=lambda node: ArrayType( | ||
| ScalarType( | ||
| node.argument_by_name("array").datatype.intrinsic, | ||
| node.argument_by_name("array").datatype.precision), | ||
| node.argument_by_name("array").datatype. | ||
| intrinsic, | ||
| node.argument_by_name("array").datatype. | ||
| precision), | ||
| [ArrayType.Extent.DEFERRED] | ||
| ), | ||
| reference_accesses=lambda node: ( | ||
|
|
@@ -4444,8 +4451,10 @@ class Intrinsic(IAttr, Enum): | |
| optional_args={}, | ||
| return_type=lambda node: ArrayType( | ||
| ScalarType( | ||
| node.argument_by_name("source").datatype.intrinsic, | ||
| node.argument_by_name("source").datatype.precision), | ||
| node.argument_by_name("source").datatype. | ||
| intrinsic, | ||
| node.argument_by_name("source").datatype. | ||
| precision), | ||
| ([ArrayType.Extent.DEFERRED] * | ||
| (len(node.argument_by_name("source").datatype.shape) + 1) | ||
| if isinstance(node.argument_by_name("source").datatype, | ||
|
|
@@ -4714,8 +4723,10 @@ class Intrinsic(IAttr, Enum): | |
| ArrayType)) | ||
| else ArrayType( | ||
| ScalarType( | ||
| node.argument_by_name("mold").datatype.intrinsic, | ||
| node.argument_by_name("mold").datatype.precision | ||
| node.argument_by_name("mold").datatype. | ||
| intrinsic, | ||
| node.argument_by_name("mold").datatype. | ||
| precision | ||
| ), | ||
| [ArrayType.Extent.DEFERRED]) | ||
| ), | ||
|
|
@@ -4737,8 +4748,10 @@ class Intrinsic(IAttr, Enum): | |
| arg_names=(("matrix",),)), | ||
| optional_args={}, | ||
| return_type=lambda node: ArrayType(ScalarType( | ||
| node.argument_by_name("matrix").datatype.intrinsic, | ||
| node.argument_by_name("matrix").datatype.precision), | ||
| node.argument_by_name("matrix").datatype. | ||
| intrinsic, | ||
| node.argument_by_name("matrix").datatype. | ||
| precision), | ||
| [node.argument_by_name("matrix").datatype.shape[1], | ||
| node.argument_by_name("matrix").datatype.shape[0]] | ||
| ), | ||
|
|
@@ -4897,7 +4910,21 @@ def datatype(self) -> DataType: | |
| if isinstance(self.intrinsic.return_type, Callable): | ||
| try: | ||
| return self.intrinsic.return_type(self) | ||
| except TypeError as err: | ||
|
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 know we discussed this before but can you remind me why we don't simply use: I know for debugging the more explicit error could help, but from a user point of view it doesn't matter that much it would be better to get the UnresolvedType to communicate that we can't do it but let it continue.
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 think I'd prefer to be more precise as from a user point of view I think it should never reach the InternalError, so its mostly being defensiveness from a development standpoint to avoid reaching things that are unexpected, but I'm happy to change it if you would prefer.
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. Ok (it still feels not ideal that we compare against the error message, hopefully we can improve on this in the furture) |
||
| # If we get an invalid argument to a ScalarType constructor it | ||
| # means we attempted to pass either an UnresolvedType into the | ||
| # datatype | ||
| if ("ScalarType expected 'intrinsic' argument to be of type " | ||
| in str(err) | ||
| or "ScalarType expected 'precision' argument to be of " | ||
| "type " in str(err)): | ||
| return UnresolvedType() | ||
| # Is this reachable? Tested via monkeypatch as there may be | ||
| # some edge case I can't think of. | ||
|
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 would prefer a comment like "This should never happen, so propagate as an InternalError" instead.
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. Yeah, thats neater, the previous comment probably should have been labelled as FIXME, updated now. |
||
| outerr = err | ||
| except AttributeError as err: | ||
| # This is to handle when we call .intrinsic or | ||
| # .precision on an UnresolvedType | ||
| # If we get an attribute error, and its because of attempting | ||
| # to lookup the precision or intrinsic, then it is likely | ||
| # due to looking up the datatype elements of an Unresolved | ||
|
|
@@ -4908,13 +4935,15 @@ def datatype(self) -> DataType: | |
| and "NoneType" not in | ||
| str(err)): | ||
| return UnresolvedType() | ||
| # Can't use debug string due to this being a potentially | ||
| # incomplete IntrinsicCall | ||
| raise InternalError( | ||
| f"Failed to compute the datatype of a " | ||
| f"'{self.intrinsic.name}' intrinsic. This is likely due " | ||
| f"to not fully initialising the intrinsic correctly." | ||
| ) from err | ||
| outerr = err | ||
| # Fall through to the internalerror. | ||
| # Can't use debug string due to this being a potentially | ||
| # incomplete IntrinsicCall | ||
| raise InternalError( | ||
| f"Failed to compute the datatype of a " | ||
| f"'{self.intrinsic.name}' intrinsic. This is likely due " | ||
| f"to not fully initialising the intrinsic correctly." | ||
| ) from outerr | ||
| else: | ||
| return self.intrinsic.return_type | ||
|
|
||
|
|
||
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.
Delete this now?
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.
Yeah.