Skip to content
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

Add transformation generated imports to driver or after inlining #321

Merged
merged 6 commits into from
Jun 13, 2024

Conversation

awnawab
Copy link
Contributor

@awnawab awnawab commented Jun 10, 2024

This PR provides the following fixes:

  1. TemporariesPoolAllocator transformation now adds global variable imports to the driver for variables used to define array sizes.
  2. HoistArraysTransformation adds imports used to define the sizes of hoisted variables.
  3. The addition of module imports from inlined children in the InlineTransformation is fixed.
  4. InlineTransformation now also adds any explicit interfaces from inlined children.

@awnawab awnawab changed the title Naan import var sizes Add transformation generated imports Jun 10, 2024
Copy link

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/321/index.html

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.16%. Comparing base (eb793e2) to head (78d9f45).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #321   +/-   ##
=======================================
  Coverage   95.15%   95.16%           
=======================================
  Files         168      168           
  Lines       35454    35503   +49     
=======================================
+ Hits        33737    33786   +49     
  Misses       1717     1717           
Flag Coverage Δ
lint_rules 96.38% <ø> (ø)
loki 95.14% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@awnawab awnawab marked this pull request as ready for review June 10, 2024 09:17
@awnawab awnawab requested review from mlange05 and reuterbal June 10, 2024 09:17
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Thanks, looks great! Some small implementation pointers but otherwise all good.

Comment on lines 145 to 146
item.trafo_data[self._key]["imported_sizes"] = [(d.type.module, d) for d in dims
if str(d) in routine.import_map]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This rebuilds the import_map in every iteration. Better to cache it before the list comprehension.

Suggested change
item.trafo_data[self._key]["imported_sizes"] = [(d.type.module, d) for d in dims
if str(d) in routine.import_map]
import_map = routine.import_map
item.trafo_data[self._key]["imported_sizes"] = [(d.type.module, d) for d in dims
if d.name in import_map]

# Add imports used to define hoisted
missing_imports_map = defaultdict(set)
for module, var in item.trafo_data[self._key]["imported_sizes"]:
if not var.name in routine.import_map:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above about caching import_map

Comment on lines 197 to 198
Resolve sequence association in calls to all member procedures (if `inline_internals = True`)
or in calls to procedures that have been marked with an inline pragma (if `inline_marked = True`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Resolve sequence association in calls to all member procedures (if `inline_internals = True`)
or in calls to procedures that have been marked with an inline pragma (if `inline_marked = True`).
Resolve sequence association in calls to all member procedures (if ``inline_internals = True``)
or in calls to procedures that have been marked with an inline pragma (if ``inline_marked = True``).

RST syntax...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, not sure how that ended up here, thanks for spotting!

Comment on lines 634 to 635
for b in intf.body:
s = getattr(b, 'procedure_symbol', None)
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 not use intf.symbols here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interface.body comprises of Subroutine objects and not just the symbols, so I did it like this to capture the whole Subroutine object. If using intf.symbols, I would have to add symbol.type.dtype.procedure to the body rather than the symbol. Either is fine by me, please let me know if you would like me to update to the latter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I'm aware - but you're only inspecting the procedure symbols of these subroutines not the objects themselves, which is what intf.symbols should give you. Have a look at the implementation of Interface.symbols, which should be essentially the same as what you do here, but also includes the case of named interfaces etc:

loki/loki/ir/nodes.py

Lines 700 to 710 in 77114a9

def symbols(self):
"""
The list of symbol names declared by this interface
"""
symbols = as_tuple(flatten(
getattr(node, 'procedure_symbol', getattr(node, 'symbols', ()))
for node in self.body # pylint: disable=not-an-iterable
))
if self.spec:
return (self.spec,) + symbols
return symbols

@awnawab
Copy link
Contributor Author

awnawab commented Jun 10, 2024

Many thanks @reuterbal for the super prompt review and feedback 🙏 I've addressed all your comments, please have another look whenever you can!

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Thanks for the quick implementation of changes! Looks good to me

@reuterbal reuterbal added the ready to merge This PR has been approved and is ready to be merged label Jun 10, 2024
@reuterbal reuterbal linked an issue Jun 10, 2024 that may be closed by this pull request
@awnawab awnawab force-pushed the naan-import-var-sizes branch from 611c17c to 0b99f61 Compare June 11, 2024 12:47
@reuterbal
Copy link
Collaborator

Apologies, this has accrued conflicts now - @awnawab, would you mind rebasing this over main?

@awnawab awnawab force-pushed the naan-import-var-sizes branch from 0b99f61 to 78d9f45 Compare June 12, 2024 20:28
@reuterbal reuterbal changed the title Add transformation generated imports Add transformation generated imports to driver or after inlining Jun 13, 2024
@reuterbal reuterbal merged commit d5a8e6c into main Jun 13, 2024
12 checks passed
@reuterbal reuterbal deleted the naan-import-var-sizes branch June 13, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add module imports to intermediate layers when hoisting temporaries
2 participants