Support optional and named arguments in inlining #2801
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2801 +/- ##
========================================
Coverage 99.88% 99.89%
========================================
Files 359 360 +1
Lines 50833 51006 +173
========================================
+ Hits 50777 50950 +173
Misses 56 56 ☔ View full report in Codecov by Sentry. |
|
Hi @arporter , this is now ready for review. It's on supporting the optional arguments using the previous pull request on the argument matching. Regarding the options dictionary, I'm now using a set_option(...) for some classes. This makes "override" options IMHO much clearer rather than using some dictionary that can have arbitrary strings in it. (@hiker , I think that's also what you wanted). |
|
This PR depends on PR #2775 |
|
@arporter @sergisiso @hiker I thought it would be good to have you all informed about this PR because of some restructuring of the callee detection and I guess this is getting more and more important for other use cases of psyclone. We have the following two different situations:
|
There was a problem hiding this comment.
Thanks very much Martin, this is exciting functionality. However, there are a couple of biggish issues that I want to tackle before I go any further with the review. First, you have implemented a different way of handling options for InlineTrans. We need project-wide agreement on how we do this so that we can adopt a uniform approach (#2668 ). We should use #2668 to come to some agreement and then you can implement that here. Second, you have cool new functionality to remove branching when the condition is a literal and I think that should be broken out as a separate transformation with an associated PR.
Finally, there are some files with non-functional changes that you can just revert to master to shrink this PR.
| @@ -195,5 +195,5 @@ | |||
| 'OMPScheduleClause', | |||
| 'OMPFirstprivateClause', | |||
| 'OMPSharedClause', | |||
| 'OMPDependClause' | |||
| ] | |||
| 'OMPDependClause', | |||
There was a problem hiding this comment.
Please revert (since there's no functional change to this file).
| @@ -0,0 +1,18 @@ | |||
|
|
|||
There was a problem hiding this comment.
Thanks for adding this. Please could you add the usual licence/copyright header (with appropriate year).
There was a problem hiding this comment.
Also, please add a comment saying it builds the html versions of all of the docs.
| make -C developer_guide html SPHINXOPTS="-W --keep-going" | ||
|
|
||
| reference_guide: | ||
| make -C reference_guide html SPHINXOPTS="-W --keep-going" |
There was a problem hiding this comment.
Unfortunately this gives:
build finished with problems, 2 warnings (with warnings treated as errors).
make[1]: *** [Makefile:19: html] Error 1
make[1]: Leaving directory '/home/me/Projects/PSyclone/doc/reference_guide'
make: *** [Makefile:9: reference_guide] Error 2
Possibly the simplest thing to do is not to "treat warnings as errors" for the Reference Guide.
There was a problem hiding this comment.
Please remove this - I don't think we need a script to wrap around pytest -n x blah. Especially since running the whole test suite takes a few minutes.
| @@ -178,7 +178,8 @@ def test_omptask_apply_kern(fortran_reader, fortran_writer): | |||
| new_container.addchild(my_test) | |||
| sym = my_test.symbol_table.lookup("test_kernel") | |||
| sym.interface.container_symbol._reference = test_kernel_mod | |||
| trans = OMPTaskTrans() | |||
| trans: OMPTaskTrans = OMPTaskTrans() | |||
| trans.set_option(check_matching_arguments_of_callee=False) | |||
There was a problem hiding this comment.
This change to the way we set options for a transformation needs to be agreed with everyone and done as a separate PR. Please revert it here.
|
|
||
| def __init__(self): | ||
| # List of call-to-subroutine argument indices | ||
| self._ret_arg_match_list: List[int] = None |
There was a problem hiding this comment.
Why is this Transformation class now holding state? I wouldn't expect this to be necessary...
| :type node: :py:class:`psyclone.psyir.nodes.Routine` | ||
| :param call_node: target PSyIR node. | ||
| :type call_node: :py:class:`psyclone.psyir.nodes.Call` | ||
| :param routine: PSyIR subroutine to be inlined. |
There was a problem hiding this comment.
What's the reason for adding this argument?
|
|
||
| # Validate that the inlining can also be accomplish. | ||
| # This routine will also update | ||
| # self.node_routine and self._ret_arg_match_list |
There was a problem hiding this comment.
Ah, so that's why there's state? Please update the comments on the declarations of these member variables.
| # This routine will also update | ||
| # self.node_routine and self._ret_arg_match_list | ||
| # with the routine to be inlined and the relation between the | ||
| # arguments and to which routine arguments they are matched to. |
There was a problem hiding this comment.
"...and which routine arguments..."
| # - `True`: Replace If-Block with If-Body | ||
| # - `False`: Replace If-Block with Else-Body. If it doesn't exist | ||
| # just delete the if statement. | ||
| self._optional_arg_eliminate_ifblock_if_const_condition() |
There was a problem hiding this comment.
This PR would solve issue #2525