(Towards #3060) reference accesses for intrinsicCall#3125
(Towards #3060) reference accesses for intrinsicCall#3125LonelyCat124 wants to merge 51 commits intomasterfrom
Conversation
LonelyCat124
changed the title
|
Follow-on to #3119 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3125 +/- ##
==========================================
- Coverage 99.95% 99.95% -0.01%
==========================================
Files 380 380
Lines 53949 53922 -27
==========================================
- Hits 53927 53900 -27
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LonelyCat124
added
ready for review
Blocked
|
@sergisiso I have a first implementation of this now if you want to examine it if you have time. Its currently "blocked" by #3119 (which is in turn dependent on #3110 ) so there's very much no rush on this, especially if I do solve some other issues before we merge 3319. |
sergisiso
left a comment
sergisiso
left a comment
There was a problem hiding this comment.
@LonelyCat124 See a few more comments, thanks for adding the new test. There is a previous comment that I haven't decided yet but I will come back to it.
src/psyclone/tests/psyir/transformations/transformations_test.py
Outdated
Show resolved
Hide resolved
| except InternalError: | ||
| # The argument here is also used in some other way | ||
| # so we do nothing as the other usage has precedence. | ||
| pass |
There was a problem hiding this comment.
I am not a fan of this InternalError, is there any reason that the functions need to guarantee the starting value. Could they not be simply \.change_to_constant() and never raise?
There was a problem hiding this comment.
I just copied the structure of the other functions here - I assume its safety to avoid it being called on the wrong things, e.g. if something is written elsewhere we can't change this to be constant as I think some of our other analysis stuff checks for if(any(type_access == CONSTANT)) and this would break a lot of that code.
There was a problem hiding this comment.
I have been thinking about these middle methods and they still don't make sense to me.
They not only make things more complicated: adding multiple layers, unexpected behaviours (e.g. fail if there is > 1 READ accesses), internal errors... And fail for simple things that should be handled, e.g. the assignment "g(g(1)) = 1" currently fails.
I believe the implementation would be easier if we just delete them and do a more explicit:
access[the_specific_access_to_update].access_type = the_new_access_type
I created #3300 to test this. The base of that PR is this branch, so if you agree you can click merge to bring the changes here.
There was a problem hiding this comment.
Merged it in.
|
@sergisiso Ready for another look when you have time, think most of the funtionality should be nearly there. |
sergisiso
left a comment
sergisiso
left a comment
There was a problem hiding this comment.
@LonelyCat124 See inline comments and proposed change
| except InternalError: | ||
| # The argument here is also used in some other way | ||
| # so we do nothing as the other usage has precedence. | ||
| pass |
There was a problem hiding this comment.
I have been thinking about these middle methods and they still don't make sense to me.
They not only make things more complicated: adding multiple layers, unexpected behaviours (e.g. fail if there is > 1 READ accesses), internal errors... And fail for simple things that should be handled, e.g. the assignment "g(g(1)) = 1" currently fails.
I believe the implementation would be easier if we just delete them and do a more explicit:
access[the_specific_access_to_update].access_type = the_new_access_type
I created #3300 to test this. The base of that PR is this branch, so if you agree you can click merge to bring the changes here.
| for ind in write_indices: | ||
| if ind < len(node.arguments) and node.argument_names[ind] is None: | ||
| arg = node.arguments[ind] | ||
| _add_write_argument(arg, reference_accesses) |
There was a problem hiding this comment.
I resolved the previous converstion because it was getting to long, but I still think it would be better to use the recursion (arg.reference_accesses()) that will properly handle all internal accesses, instead of thinking in each case what can or cannot happen in order to come up with the method implementation. Then all the add mehtods will be the same and can be implemented with a single method doing:
accesses = arg.reference_accesses()
if isinstance(arg, Reference):
sig, _ = arg.get_signature_and_indices()
accesses[sig][-1].access_type = access_type
with arg and access_type as a parameter. (note that I am using the setter from the proposed PR in the previous comment)
Or even inlining this expression.
There was a problem hiding this comment.
I created a single function to do this and tests seemed ok with it.
| reference_accesses = VariablesAccessMap() | ||
| # For indices, we only apply them if there is no argument name, othrwise | ||
| # it should be handled by the argument names. | ||
| for ind in read_indices: |
There was a problem hiding this comment.
This would make the AccessSequence have a strange order instead of left-to-right arguments. Tbh I don't think there is a "right" order, but it may be neater to start with a:
for ind, arg in enumerate(node.arguments)
and then do each individual:
if ind in read_indices ....
if node.argument_names[ind] in read_named_args ...
to end up with a more sensible order
(towards #3060) Remove AccessSequence/Type methods
…to 3060_reference_accesses
|
@sergisiso Ready for a review when you're back from leave. |
2 participants
No description provided.