Fix function inlining when only interface is available (fixes #397) #402
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/402/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #402 +/- ##
==========================================
+ Coverage 95.57% 95.62% +0.04%
==========================================
Files 201 201
Lines 39810 39891 +81
==========================================
+ Hits 38050 38146 +96
+ Misses 1760 1745 -15
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.
Ok, not a fan but since this only increases test coverage, it's ready to go.
As a side-note, I think we need a separate re-write of transformation.inline module, as the new capabilities from PR #378 basically voided the previous separation by callee-type (which was an artifact of only allowing single-line substitutions). The module now also has cyclic references and needs an general overhaul, I think, but that beyond this PR.
| assignments = FindNodes(ir.Assignment).visit(routine.body) | ||
|
|
||
| if provide_myfunc in ('import', 'module', 'intfb'): | ||
| assert len(inline_calls) == 0 # MYFUNC(arr) is misclassified as array subscript or fully inlined |
There was a problem hiding this comment.
[no action] Really? We're misqualifying the inline call within statement functions? Is this a known bug?
There was a problem hiding this comment.
It's not a bug, it's a lack of information. There are two cases here, which could maybe be made more explicit in the test:
if provide_myfunc in ('import, 'intfb'): # We only have the use statement or nothing but the C-include
assert len(inline_calls) == 0 # MYFUNC(arr) is misclassified as array subscript
elif provide_myfunc == 'module': # We have the full function definition
assert len(inline_calls) == 0 # MYFUNC(arr) is eliminated by inlining
In the first case we have that symbol but don't have the interprocedural annotation that explains what this symbol is. Which brings us to the ambiguity of fortran that does not syntactically allow to distinguish this call from an array subscript and fparser classifies it as the latter.
In the second case we eliminate MYFUNC.
There was a problem hiding this comment.
Just to add, it's independent from the statement function - the same is true in any other expression
| assert assignments[0].lhs == 'ret' | ||
| assert assignments[1].lhs == 'ret2' | ||
| if provide_myfunc == 'module': | ||
| assert assignments[0].rhs == "arr + arr + 1.0 + arr*2.0 + arr*2.0" |
There was a problem hiding this comment.
[no action] This highlights that the entire separation of inline_statement_function and inline_elemental is basically now broken. All four definition types should yield consistent behaviour.
There was a problem hiding this comment.
No, the behaviour difference comes again from the lack of information. Inlining always works the same if you have the definition of the function body available, and fails when you don't have it
In fact, let me add a fifth case provide_myfunc == 'subroutine', where we have the intfb and provide the definition separately as a free function (not embedded in a module). Providing this as a definition should also allow the inlining to be effective
mlange05
added
the
ready for merge
|
Thanks for the review and the helpful comments. I have added a few more explanations to the test and expanded this to include the case that the function is provided via the definitions. In that situation we should also have all the information available to inline the body. It turns out that we don't use this information in the fparser frontend, though, so I added a small fix for this. |
reuterbal
deleted the
397-improve-test-coverage-for-inlinesubstitutionmapper
branch
This adds the missing test for
InlineSubstitutionMapper, expanded to four different cases:This includes a small fix to the mapper for the third case, where procedure information is available but that referes only to the interface, i.e., no body for inlining is present.
Let's see what codecov says...