Fixes to minor issues related to SCC HOIST #211
Conversation
… move sccannotate one step further, to also add acc present statements for scc hoisted arrays
| @@ -150,6 +151,8 @@ def transform_subroutine(self, routine, **kwargs): | |||
| call_map = CaseInsensitiveDict((str(call.name), call) for call in calls) | |||
|
|
|||
| for child in successors: | |||
| if not isinstance(child, SubroutineItem): | |||
| continue | |||
| arg_map = dict(call_map[child.local_name].arg_iter()) | |||
There was a problem hiding this comment.
If SCC HOIST is used together with GlobalVarOffloadTransformation, some of successors may be Items (corresponding to the imported global variables). Therefore, adding this if statement to skip processing for Items other than SubroutineItems. To me, everything here seems to indicate this loop only processes subroutines.
| scheduler.process( SCCAnnotateTransformation( | ||
| horizontal=horizontal, vertical=vertical, directive=directive, block_dim=block_dim | ||
| )) | ||
|
|
||
| if mode in ['cuf-parametrise', 'cuf-hoist', 'cuf-dynamic']: |
There was a problem hiding this comment.
Notice that this change moves the location of SCCAnnotate "one step" further in the SCC pipeline. The reasoning for this is that SCCHoistTemporaryArraysTransformation does the SCC hoisting, introducing new arguments to subroutines, that have to be marked as "present", This change ensures that SCCAnnotate adds these pragmas for the new arguments as well.
I am not sure whether SCCAnnotate should actually be moved after the next if statement even.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #211 +/- ##
=======================================
Coverage 92.26% 92.26%
=======================================
Files 96 96
Lines 17124 17131 +7
=======================================
+ Hits 15799 15806 +7
Misses 1325 1325
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks, this looks great to me.
I let @mlange05 assess whether moving the Annotate transformation is safe to do but in principle I think it would be great to have this later in the pipeline and develop it into a more generic offload annotation mechanism that will eventually be able to take on that duty for other transformations and programming models as well.
Otherwise, I have left two remarks. One of them relates to a suggestion to capture the original issue in the test base to ensure we don't re-introduce that bug in the future.
scripts/loki_transform.py
Outdated
| scheduler.process( SCCAnnotateTransformation( | ||
| horizontal=horizontal, vertical=vertical, directive=directive, block_dim=block_dim | ||
| )) |
There was a problem hiding this comment.
I believe we should guard this as before:
| scheduler.process( SCCAnnotateTransformation( | |
| horizontal=horizontal, vertical=vertical, directive=directive, block_dim=block_dim | |
| )) | |
| if mode in ['scc', 'scc-hoist', 'scc-stack']: | |
| scheduler.process( SCCAnnotateTransformation( | |
| horizontal=horizontal, vertical=vertical, directive=directive, block_dim=block_dim | |
| )) |
There was a problem hiding this comment.
I agree, this is a good idea. I changed this.
| @@ -150,6 +151,8 @@ def transform_subroutine(self, routine, **kwargs): | |||
| call_map = CaseInsensitiveDict((str(call.name), call) for call in calls) | |||
|
|
|||
| for child in successors: | |||
| if not isinstance(child, SubroutineItem): | |||
There was a problem hiding this comment.
This is currently not covered in the test base, which is why this bug existed in the first place.
I would suggest to amend the existing test test_single_column_coalesced_hoist_openacc by moving the integer c to a new module and importing it in the kernel.
Here's an untested implementation suggestion:
fcode_module = """
module my_scaling_value_mod
implicit none
REAL :: c = 5.345
end module my_scaling_value_mod
""".strip()
[...]
module_source = Sourcefile.from_source(fcode_module, frontend=frontend)
module = module_source['my_scaling_value_mod']
[...]
kernel.enrich(module) # do this before enriching the driver
[...]
module_item = GlobalVarImportItem(name='my_scaling_value_mod#c', source=module_source)
[...]
# This replaces the current implementation
for transform in scc_transform:
transform.apply(driver, role='driver', item=driver_item, targets=['compute_column'], successors=[kernel_item])
transform.apply(kernel, role='kernel', item=kernel_item), successors=[module_item])There was a problem hiding this comment.
Thanks, this was a good idea.
I changed the test to the best of my ability, please check to see it is OK. I had to add the successors argument to the analysis part (a bit later than you suggested). I ran the modified test in the main branch to make sure that it fails there.
…alysis and synthesis
|
Thanks for reviewing @reuterbal. |
reuterbal
added
the
ready to merge
Hello.
This PR contains two very small changes. I discuss them below.