-
Notifications
You must be signed in to change notification settings - Fork 28
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
(Towards #2704) 2704 access improvements #2734
base: master
Are you sure you want to change the base?
Conversation
…uctures is handled
…esses in the else block
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2734 +/- ##
========================================
Coverage 99.86% 99.86%
========================================
Files 354 355 +1
Lines 48910 49166 +256
========================================
+ Hits 48846 49102 +256
Misses 64 64 ☔ View full report in Codecov by Sentry. |
I see this is already quite big, could separate them in different PRs, this one, next one for backwards and next one to replace forward/backward_dependence ? At this point we just need to know that there is a path/plan to replace forward/backward_dependence with the new functionality and we don't end up with multiple implementations of very similar things. What do you think? |
Yeah that sounds fine! I'll fix coverage and then put this bit up for ready for review. |
@sergisiso @arporter This is ready for a first look now. I'm editing it to towards 2704 now as its only the first piece (but as Sergi says, already big enough to be its own PR). |
So from some experimentation - in principle the functionality of There are 23 failing tests with this implementation:
I think this code matches the previous code, however I can't yet check because Edit: Actually its more complex than this - at the moment this depends on some subclass implementations in psyGen.py which use _find_arguments and following, and this |
Can you fix the code formatting, it didn't get the linebreaks.
Ah I forgot about it, it won't be easy but I think I have a long term plan. We need the new backend (soon), moving the invoke.declaration that creates the symbol at the raising step (psyir construction), and let the argument know its symbol (they can subclass reference then). But that won't be ready anytime soon, so let's forget about replacing those methods for now. |
Done, sorry my windows terminal just stops copying line breaks. |
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.
@LonelyCat124 I made a first high-level review, I like the structure/interface, I just made minor comments about it. I have not gone too deep into the implementation details yet.
I noticed there are a few FIXMEs, TODOs in the code. Can some of this be resolved now?
Also it will be good to document in https://psyclone-dev.readthedocs.io/en/latest/dependency.html this new class, describe its functionality and how to use it, and how is it different from the others tools/methods mentioned in that same page.
@property | ||
def is_read(self): | ||
''' | ||
:returns: whether this reference is a read to its symbol. |
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.
Maybe: whether this reference is reading from its symbol.
if isinstance(parent, Call): | ||
# For now we assume that all arguments of a Call (or non- | ||
# inquiry or pure IntrinsicCalls) are read (they may also be | ||
# written to. This can be improved in the future by looking | ||
# at intents. | ||
return True |
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.
Do we need this and the Call import at all if we return True anyway? Maybe remove it and just update the comment below to:
# All references other than LHS of assignments represent a read, this can be
# improved in the future by looking at Call intents.
@property | ||
def is_write(self): | ||
''' | ||
:returns: whether this reference is a write to its symbol. |
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.
Maybe: whether this reference is writing to its symbol.
def next_access(self): | ||
''' | ||
:returns: the next reference to the same symbol. | ||
:rtype: Optional[:py:class:`psyclone.psyir.nodes.Node`] | ||
:returns: the following References to the same symbol. There may be | ||
multiple References returned as control flow may result | ||
in many possible next_accesses and all may need to be | ||
checked. | ||
:rtype: List[:py:class:`psyclone.psyir.nodes.Node`] |
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.
Since we return a list, I would rename the method to "next_accesses".
And for the docstring, what do you think of:
the nodes accessing the same symbol directly after this reference. It can be multiple nodes if the control flow diverges and there are multiple possible accesses.
# The scope is as far as the Routine that contains this | ||
# Reference. | ||
routine = self.ancestor(Routine) | ||
# Handle the case when this is a subtree without an ancestor | ||
# Routine | ||
if routine is None: | ||
routine = self.root |
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.
Do we need this code and passing the [routine]
argument? I think this is the same that the DefinitionUseChain does by default.
""" | ||
:returns: the list of output nodes computed by this DefinitionUseChain. | ||
:rtype: List[:py:class:`psyclone.psyir.nodes.Node`] | ||
""" | ||
return self._defsout | ||
|
||
@property | ||
def killed(self): | ||
""" | ||
:returns: the list of killed output nodes computed by this | ||
DefinitionUseChain. | ||
:rtype: List[:py:class:`psyclone.psyir.nodes.Node`] | ||
""" | ||
return self._killed |
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.
Can you expand what defsout are, I don't quite understand what they are and how are they different than killed?
basic_block_list provided. This function will not work | ||
correctly if there is control flow inside the | ||
basic_block_list provided. | ||
FIXME should we check and raise an exception? |
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.
Maybe not since this is a private method. You can remove this FIXME
# TODO For now the if statement doesn't kill the accesses, | ||
# even though it will always be written to. | ||
# FIXME Add issue to the previous comment |
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.
Yes please, create and reference in the TODO the issue for this.
# TODO I don't like this, can we instead expand this when placing it | ||
# in? At the moment if we do this it breaks the is_basic_block check | ||
# as we end up with a list inside a list. | ||
# Maybe this is ok now? |
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.
Has this been solved?
# FIXME If all defsout is in control flow we should add a None into | ||
# the defsout array. @Reviewer not sure about this - should we make | ||
# it clear somehow its not guaranteed to be written to or does it not | ||
# matter? |
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.
I am not sure about this, maybe we need to discuss this again with some examples/use_cases of how this will be used?
WIP.
Forward_access implementation is done. I need to do backwards next.
@sergisiso How should we modify forward/backward_dependence from node? Do these need to only return 1 result or should we change them to return potentially multiple?