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

Port over loop handling utils from PSyACC #2754

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jwallwork23
Copy link
Collaborator

@jwallwork23 jwallwork23 commented Oct 23, 2024

Partially addresses #2717.

This PR merges functions from the loop module of PSyACC into PSyclone:

  • is_outermost checks that the Loop doesn't have any ancestors which are Loops.
  • is_perfectly_nested checks that the Loop nest starting at a given Loop is "loops all the way down" until the last.
  • is_simple checks that a Loop nest is perfect and that the deepest level only contains literal assignments (i.e., the loop nest is trivially parallelisable).
  • nest_variable_names extracts the tuple of variable names for the Loop nest starting at a given Loop.

@jwallwork23 jwallwork23 added the NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH label Oct 23, 2024
@jwallwork23 jwallwork23 self-assigned this Oct 23, 2024
@sergisiso
Copy link
Collaborator

@jwallwork23 Isn't this equivalent to the Loop.independent_iterations method? (I think we considered this name for that method but decided the independent iterations could be more generic)

@jwallwork23
Copy link
Collaborator Author

@jwallwork23 Isn't this equivalent to the Loop.independent_iterations method? (I think we considered this name for that method but decided the independent iterations could be more generic)

Oh good, if there is already an equivalent method then that's one less thing to merge in :) Thanks!

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (8367de1) to head (90bc743).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2754   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         354      354           
  Lines       49010    49044   +34     
=======================================
+ Hits        48946    48980   +34     
  Misses         64       64           

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

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Thanks @jwallwork23. I have a couple of questions about two of the new methods so I haven't gone on to look at the tests yet.
Ref. guide builds cleanly though.

@@ -43,6 +44,10 @@
from psyclone.psyir.nodes.statement import Statement
from psyclone.psyir.nodes.routine import Routine
from psyclone.psyir.nodes import Schedule
from psyclone.psyir.nodes.assignment import Assignment
Copy link
Member

Choose a reason for hiding this comment

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

Please put these imports in alpha-order with the other ones (ordered by module name).

'''
return [item for item in list1 if item in list2]

# Check whether the subnest is perfect by checking each level in turn
Copy link
Member

Choose a reason for hiding this comment

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

This looks pretty complicated. I think you'd be better off using the loop_body property of Loop rather than doing a walk. Could you use/adapt the implementation at:

num_collapsable_loops = 0
next_loop = node
previous_iteration_variables = []
while isinstance(next_loop, Loop):
previous_iteration_variables.append(next_loop.variable)
num_collapsable_loops += 1
if not isinstance(collapse, bool):
if num_collapsable_loops >= collapse:
break
# If it has more than one child, the next loop will not be
# perfectly nested, so stop searching. If there is no child,
# we have an empty loop and we also stop here.
if len(next_loop.loop_body.children) != 1:
if (next_loop.loop_body.children and
isinstance(next_loop.loop_body[0], Loop)):
next_loop.loop_body[0].append_preceding_comment(
"Loop cannot be collapsed because it has siblings")
break
next_loop = next_loop.loop_body[0]
if not isinstance(next_loop, Loop):
break
# If it is a loop dependent on a previous iteration variable
# (e.g. a triangular iteration space), it can not be collapsed
dependent_on_previous_variable = False
for bound in (next_loop.start_expr, next_loop.stop_expr,
next_loop.step_expr):
for ref in bound.walk(Reference):
if ref.symbol in previous_iteration_variables:
dependent_on_previous_variable = ref.symbol
break
if dependent_on_previous_variable:
if verbose:
next_loop.append_preceding_comment(
f"Loop cannot be collapsed because one of the "
f"bounds depends on the previous iteration variab"
f"le '{dependent_on_previous_variable.name}'")
break
# Check that the next loop has no loop-carried dependencies
dtools = DependencyTools()
if not ignore_dep_analysis:
if not next_loop.independent_iterations(
dep_tools=dtools,
signatures_to_ignore=list_of_signatures):
if verbose:
msgs = dtools.get_all_messages()
next_loop.preceding_comment = (
"\n".join([str(m) for m in msgs]) +
" Consider using the \"ignore_dependencies_"
"for\" transformation option if this is a "
"false dependency.")
break

(and then replace/update that code to use this new method)?

return True

@property
def is_simple(self):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this one. It seems very niche (why assignments only with Literals?). How does the use case compare with that for Loop.independent_iterations?

'''
:returns: Names of all variables within the Loop nest of descendents
of this Loop, inclusive.
:rtype: tuple
Copy link
Member

Choose a reason for hiding this comment

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

tuple[str] please.

from psyclone.psyir.nodes.assignment import Assignment
from psyclone.psyir.nodes.intrinsic_call import IntrinsicCall
from psyclone.psyir.nodes.literal import Literal
from psyclone.psyir.nodes.reference import Reference
from psyclone.psyir.symbols import ScalarType, DataSymbol
from psyclone.core import AccessType, Signature
Copy link
Member

Choose a reason for hiding this comment

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

Not yours but would you mind putting all of these in alpha order in fact?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH reviewed with actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants