(Closes #3334) Make module-inlining mandatory for Kernel transformations#3388
(Closes #3334) Make module-inlining mandatory for Kernel transformations#3388
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3388 +/- ##
==========================================
- Coverage 99.95% 99.95% -0.01%
==========================================
Files 387 388 +1
Lines 54310 54202 -108
==========================================
- Hits 54288 54180 -108
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
ITs were all green and coverage is now OK. Ready for a first review from @hiker, @LonelyCat124 or @sergisiso. |
|
I got worried that the previous PR which still had several open comments ended saying it was merged: But what happened is that those commits were part of the Anyway I've checked that the comments I made in the previous PR are now addressed or ported to the new review. |
| --kernel-renaming {multiple,single} | ||
| (psykal mode) naming scheme to use when re-naming transformed kernels |
There was a problem hiding this comment.
psyclone -h still has this flag because the changes to generator.py has not been ported to this second take branch.
There are also other kernel-renaming mentions here:
doc/user_guide/transformations.rst
582:two different kernel-renaming schemes: "multiple" and "single"
583:(specified via the ``--kernel-renaming`` command-line flag). In the
src/psyclone/configuration.py
742: kernel-renaming scheme.
src/psyclone/generator.py
239: :raises GenerationError: if an invalid kernel-renaming scheme is specified.
276: f"Invalid kernel-renaming scheme supplied: {str(verr)}") from verr
534: '--kernel-renaming', default="multiple",
src/psyclone/tests/generator_test.py
1681: kernel-renaming scheme is supplied.
1693: assert "Invalid kernel-renaming scheme supplied" in str(err.value)
| for kernel in kernels: | ||
| try: | ||
| kernel.check_outer_scope_accesses( | ||
| node, "Kernel", | ||
| permit_unresolved=False, | ||
| ignore_non_data_accesses=True) | ||
| except SymbolError as err: | ||
| raise TransformationError( | ||
| f"Cannot apply {self.name} to Kernel '{node.name}' " | ||
| f"because it accesses data from its outer scope: " | ||
| f"{err.value}") from err |
There was a problem hiding this comment.
I am not sure we can delete this.
You added the comment "This also implies that there are no unqualified imports or undeclared symbols.", do you mean that module-inlining would have failed? Can we not have a pre-existing local routine with unqualified imports?
Or a kernel that was module-inlined, then another transformation or script adds an unqualifired import, then we apply this.
| assert isinstance(kschedule.symbol_table.lookup("xstart").interface, | ||
| ArgumentInterface) | ||
| assert isinstance(kschedule.symbol_table.lookup("xstop").interface, | ||
| assert isinstance(kschedule.symbol_table.lookup("xstop_1").interface, |
There was a problem hiding this comment.
Why did this name change?
| self._check_callee_implementation_is_local(node) | ||
|
|
||
| def apply(self, node, **kwargs): | ||
| ''' |
There was a problem hiding this comment.
Maybe add "Do nothing" here and "Call CalleeTransformationMixin validation" in the validate docstring.
2 participants
A second attempt at this after my git fu let me down badly.
Original changes are here: https://github.com/stfc/PSyclone/pull/3294/changes#diff-436bf7f3b5f30b4c1d6ff1f9a0a8e144f924bae78cbcc9595b30fc5954e715e0