HoistVariablesAnalysis: remove unused explicit interfaces after inlining
#319
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/319/index.html |
awnawab
force-pushed
the
naan-hoist-after-inline
branch
from
067b768 to
a66737f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #319 +/- ##
==========================================
+ Coverage 95.15% 95.16% +0.01%
==========================================
Files 168 168
Lines 35454 35538 +84
==========================================
+ Hits 33737 33821 +84
Misses 1717 1717
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi. Can I push a small update here? |
awnawab
force-pushed
the
naan-hoist-after-inline
branch
from
a66737f to
be5a4f9
Compare
reuterbal
requested changes
Jun 10, 2024
| @@ -152,6 +152,11 @@ def transform_subroutine(self, routine, **kwargs): | |||
| for child in successors: | |||
| if not isinstance(child, ProcedureItem): | |||
| continue | |||
|
|
|||
| # skip successors that might have been inlined | |||
| if not call_map.get(child.local_name, None): | |||
There was a problem hiding this comment.
Shouldn't that simply be
Suggested change
| if not call_map.get(child.local_name, None): | |
| if child.local_name not in call_map: |
There was a problem hiding this comment.
As discussed offline, this can be dropped now because InlineTransformation.creates_items = True if inline_marked = True.
loki/transformations/inline.py
Outdated
| @@ -606,6 +606,18 @@ def inline_marked_subroutines(routine, allowed_aliases=None, adjust_imports=True | |||
| # Remove import if no further symbols used, otherwise clone with new symbols | |||
| import_map[impt] = impt.clone(symbols=new_symbols) if new_symbols else None | |||
|
|
|||
| # Remove explicit interfaces of inlined routines | |||
| for intf in FindNodes(Interface).visit(routine.ir): | |||
There was a problem hiding this comment.
Do we really need to go over routine.ir here? Or couldn't we simply use
Suggested change
| for intf in FindNodes(Interface).visit(routine.ir): | |
| for intf in routine.interfaces: |
loki/transformations/inline.py
Outdated
Comment on lines
612
to
615
| _body = [] | ||
| for s in intf.symbols: | ||
| if s.name not in callees or s.name in not_inlined: | ||
| _body += [s.type.dtype.procedure,] |
There was a problem hiding this comment.
This could be done with a list comprehension:
Suggested change
| _body = [] | |
| for s in intf.symbols: | |
| if s.name not in callees or s.name in not_inlined: | |
| _body += [s.type.dtype.procedure,] | |
| _body = tuple( | |
| s.type.dtype.procedure for s in intf.symbols | |
| if s.name not in callees or s.name in not_inlined | |
| ) |
Also allows to directly build a tuple, avoiding the as_tuple further down.
awnawab
force-pushed
the
naan-hoist-after-inline
branch
from
410e3c5 to
714075a
Compare
reuterbal
approved these changes
Jun 11, 2024
reuterbal
changed the title
HoistVariablesAnalysis and remove unused explicit interfaces after inliningHoistVariablesAnalysis: remove unused explicit interfaces after inlining
reuterbal
added
the
ready for merge
mlange05
approved these changes
Jun 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This small PR implements two bug fixes:
HoistVariablesAnalysisnow skips successors that might no longer be present (e.g. they were inlined)