SCCHoist: hoist inline call temporaries and don't hoist statically declared arrays #268
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/268/index.html |
| else: | ||
| new_args = tuple(v.clone(dimensions=None) for v in variables) | ||
| vmap = {call: call.clone(parameters=call.parameters + new_args)} | ||
| routine.body = SubstituteExpressions(vmap).visit(routine.body) |
There was a problem hiding this comment.
You could move routine.body = SubstituteExpressions(vmap).visit(routine.body) outside the if/else statement ...
There was a problem hiding this comment.
Ah yes, good spot, thanks!
reuterbal
force-pushed
the
naan-scc-hoist
branch
from
c1cb682
to
8e286d0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
=======================================
Coverage 92.88% 92.88%
=======================================
Files 102 102
Lines 18253 18270 +17
=======================================
+ Hits 16954 16971 +17
Misses 1299 1299
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| new_args = dict((a.name, v.clone(dimensions=None)) for (a, v) in variables) | ||
| _call_clone = call.clone(kwparameters=call.kw_parameters) | ||
| _call_clone.kw_parameters.update(new_args) |
There was a problem hiding this comment.
I find this pattern of in-place updating the call attribute a little weird, particularly since we expect to only add new kwargs to the end. Wouldn't something more similar to the other argument_remapping method be a little more intuitive?
Also, the clone call has a typo (kwparameters vs. kw_parameters), which suggests we don't have a test with existing kw_parameters :) Could you add one for that?
| new_args = dict((a.name, v.clone(dimensions=None)) for (a, v) in variables) | |
| _call_clone = call.clone(kwparameters=call.kw_parameters) | |
| _call_clone.kw_parameters.update(new_args) | |
| kw_params = call.kw_parameters | |
| kw_params += tuple((a.name, v.clone(dimensions=None)) for (a, v) in variables) | |
| _call_clone = call.clone(kwparameters=dict(kw_params)) |
reuterbal
added
the
ready for merge
| @@ -240,7 +243,7 @@ def transform_subroutine(self, routine, **kwargs): | |||
| routine.arguments += hoisted_temporaries | |||
|
|
|||
| call_map = {} | |||
| for call in FindNodes(CallStatement).visit(routine.body): | |||
| for call in FindNodes(CallStatement).visit(routine.body) + list(FindInlineCalls().visit(routine.body)): | |||
There was a problem hiding this comment.
Both of these should return tuples, I think. Better to fix the return of the visitor than to cast here, no? Or am I missing something?
There was a problem hiding this comment.
We have a bit of a mismatch there, something we should clean up separately:
FindNodeshas indeed always returned lists. Since these are built incrementally and can be quite long, there could indeed a performance impact from using tuples- Expression visitors return by default sets, unless
unique=Falseis used.
This PR adds the capability to hoist temporaries declared in inline function calls. It also changes the default behaviour so that statically declared arrays are no longer hoisted.