-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix function inlining when only interface is available (fixes #397) #402
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[no action] Really? We're misqualifying the inline call within statement functions? Is this a known bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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...