(Towards #2704) 2704 access improvements

[LonelyCat124]

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?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (e615b2b) to head (42bcc58).

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.
📢 Have feedback on the report? Share it here.

[sergisiso]

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?

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?

[LonelyCat124]

Yeah that sounds fine! I'll fix coverage and then put this bit up for ready for review.

[LonelyCat124]

@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).

[LonelyCat124]

So from some experimentation - in principle the functionality of forward_dependence wouldn't be difficult to implement, however the current DefinitionUseChains won't support it.

There are 23 failing tests with this implementation:

        # The scope is this node's parent's children.
        scope = self.parent.children[:]
        dependence = None
        for arg in self.args:
            chain = DefinitionUseChain(
                    arg,
                    scope
            )
            results = chain.find_forward_accesses()
            for res in results:
                if res.abs_position <= arg.abs_position:
                    continue
                node = res
                while node.depth > self.depth:
                    node = node.parent
                if self.sameParent(node) and node is not self:
                    # The remote node (or one of its ancestors) shares
                    # the same parent as me (but its not me)
                    if not dependence:
                        # this is the first dependence found so keep it
                        dependence = node
                    else:
                        if dependence.position > node.position:
                            # the new dependence is closer to me than
                            # the previous dependence so keep it
                            dependence = node
        return dependence

I think this code matches the previous code, however I can't yet check because DefinitionUseChains only support Reference inputs, but the forward_dependence passes DynKernelArguments instead of Reference objects. I'm not sure how hard it would be to adjust this/whether KernelArgument have any similarities to References - if not it might not help with reimplementing this.

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 _depends_on routine. I think in principle this may all be replaced by the implementation above, but it does depend on Argument vs Reference.

[sergisiso]

There are 23 failing tests with this implementation:

Can you fix the code formatting, it didn't get the linebreaks.

but the forward_dependence passes DynKernelArguments instead of Reference objects

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.

[LonelyCat124]

Can you fix the code formatting, it didn't get the linebreaks.

Done, sorry my windows terminal just stops copying line breaks.

[sergisiso]

@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.

[sergisiso]

Maybe: whether this reference is reading from its symbol.

[sergisiso]

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.

[sergisiso]

Maybe: whether this reference is writing to its symbol.

[sergisiso]

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.

[sergisiso]

Do we need this code and passing the [routine] argument? I think this is the same that the DefinitionUseChain does by default.

[sergisiso]

Can you expand what defsout are, I don't quite understand what they are and how are they different than killed?

[sergisiso]

Maybe not since this is a private method. You can remove this FIXME

[sergisiso]

Yes please, create and reference in the TODO the issue for this.

[sergisiso]

Has this been solved?

[sergisiso]

I am not sure about this, maybe we need to discuss this again with some examples/use_cases of how this will be used?