Scheduler.process: apply directly to program unit corresponding to Item

[reuterbal]

This is part 2 to #136 (and filed against that PR branch to allow consolidating the fix if acceptable).

The extended fix in this PR is more invasive than the minimum workaround implemented in #136 by applying transformations directly to the program unit corresponding to an item instead of relying on recursion to eventually end up at the right object. This also removes the need for the check of item.name == routine.name before applying transformations, because now we do actually apply transformations only directly to the subroutines that are meant to be transformed.

This highlighted also a flaw in the current scheduler design which effectively preempts processing contained member routines entirely, because we can't represent contained member routines as scheduler graph nodes (the discovery mechanism is unable to find member routines), and the above mentioned name check preempts recursion into the member routine. The implementation in this PR partially resolves this by getting rid of the name check. Proper representation of contained member routines requires more changes, though, and I will create a separate issue for that.

As this change is more invasive in the scheduler, it may have unintended side-effects and therefore this fix is optional.

[github-actions]

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

[codecov]

Codecov Report

Merging #137 (d4c926c) into 131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines (9fd7318) will decrease coverage by 0.03%.
The diff coverage is 95.23%.

@@                                                                Coverage Diff                                                                 @@
##           131-calling-a-transformation-on-a-subroutine-automatically-applies-itself-recursively-on-contained-subroutines     #137      +/-   ##
==================================================================================================================================================
- Coverage                                                                                                           92.10%   92.08%   -0.03%     
==================================================================================================================================================
  Files                                                                                                                  89       89              
  Lines                                                                                                               16515    16542      +27     
==================================================================================================================================================
+ Hits                                                                                                                15211    15232      +21     
- Misses                                                                                                               1304     1310       +6     

Flag	Coverage Δ
lint_rules	96.13% <ø> (ø)
loki	92.03% <95.23%> (+<0.01%)	⬆️
transformations	91.46% <ø> (-0.22%)	⬇️

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

Files Changed	Coverage Δ
loki/transform/transformation.py	91.89% <ø> (-0.22%)	⬇️
loki/bulk/scheduler.py	80.81% <71.42%> (-0.27%)	⬇️
loki/transform/dependency_transform.py	97.07% <100.00%> (+0.47%)	⬆️

... and 2 files with indirect coverage changes

[reuterbal]

Waiting for CI to confirm but I think I fixed the issue that was flagged by the ecwam regression test: The DependencyTransformation had an implicit dependency on the recursion behaviour of the Transformation class - both in terms of requiring the recursion to always start from the source file level, and trusting that recursion never actually reaches contained member procedures...

I added some safe guards to the transformation that ensure existing behaviour is retained.

@mlange05: Please let me know if you would like to review this as a diff to #136, or want me to merge this PR into the other and review them together?

[mlange05]

Ok, I've had a first look, and I think the final review should be on the combined PR for both. So I'm approving this now, but will take another look at the full thing.

I've already flagged a few things that strike me as odd in the dependency transform. This could very well be me not understanding some subtle detail though, so I'll need to play around with this myself a bit more.

Reading through this, I'm also not quite convinced anymore that we even need the recurse_to_contained flag. I think the only use for this were member subroutines and the sourcefile->module->subroutine cascade which we're trying to suprpess with this, no? For the member-routine recurision (a rare case, I guess), an explicit recurse_to_member_routines or even a user-level recursive call to transform_subroutine might actually be better, no?

Anyway let's merge this into #136 and see if we can whittle this down some more from there.

[mlange05]

[no action] Yes it should, but I think this requires the SGraph from the scheduler rewrite to be done cleanly.

[reuterbal]

Indeed

[mlange05]

I don't quite understand why this is necessary. This snippet here suggests we're updating the item by finding the item in items that this subroutine belongs to. I don't see why this is necessary if we only apply transform_subroutine methods to the subroutine object associated to the given item. Am I missing something here?

[reuterbal]

It's due to the fact that the cleanest way of calling this transformation is via the file graph and then using the transformation's recursion to trickle down into the various nodes. However, with that the item property isn't updated cleanly for individual program units within the source file. This leads to outdated targets and, in a situation where driver and kernel would sit in the same module, we would assume the role of one of them for both.

[reuterbal]

As requested, merged into #136 for further review.