Support optional and named arguments in inlining#2801
Support optional and named arguments in inlining#2801schreiberx wants to merge 25 commits intomasterfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2801 +/- ##
========================================
Coverage 99.89% 99.89%
========================================
Files 362 363 +1
Lines 51685 51858 +173
========================================
+ Hits 51633 51806 +173
Misses 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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:
|
arporter
left a comment
arporter
left a comment
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.
| '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.
There was a problem hiding this comment.
I'm working on the proposed changes quite late. There have been huge updates on this Makefile. I'll remove my suggested changes since the new version from master is much better.
| 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.
I'm working on the proposed changes quite late. There have been huge updates on this Makefile. I'll remove my suggested changes since the new version from master is much better.
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.
| 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.
There was a problem hiding this comment.
OK. I'll open first an issue and then we'll see. Not sure if I can do this in the near future.
|
@arporter I'll close this one and reopen another one. |
|
For reference, this is the new PR: #3067 |
2 participants
This PR would solve issue #2525