(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.87%. Comparing base (33ff7e8) to head (5c29b70).
Report is 104 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2734    +/-   ##
========================================
  Coverage   99.86%   99.87%            
========================================
  Files         354      355     +1     
  Lines       48967    49229   +262     
========================================
+ Hits        48903    49165   +262     
  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]

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

[LonelyCat124]

We don't need it - i kinda preferred being specific but I can remove it if you'd prefer.

[sergisiso]

yeah, I think I prefer removing it in this case, as it also allows to remove one of the inlined imports which we are trying to reduce

[sergisiso]

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

[LonelyCat124]

I expanded on killed to make it hopefully clearer. Realistically these are not too common in this implementation, since we usually stop caring about things once we find a write, but they can appear when we have control flow.

[LonelyCat124]

Ok - so the implementation does need/use the killed accesses now, I need some more tests for behviour inside control flow for codeblocks/calls to ensure this is behaving as I expect.

[sergisiso]

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

[LonelyCat124]

Done.

[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?

[LonelyCat124]

We discussed this previously when I was on-site - and initially thought this might be a good idea, but I'm not actually sure.
It could be left up to the code that uses the result to check this, as I think some of use-cases will not care if the write is "guaranteed" or not.

[LonelyCat124]

I think also until #2760 is implemented it doesn't make sense to fix this anyway.

[LonelyCat124]

I've added a small section to the documentation, it probably needs expanding but a bit unsure what to put in there for now.

TODO: Handle CYCLE, BREAK, GOTO etc.

[LonelyCat124]

ok - there is an issue with how i'm handling writes to the reference that I need to fix. I produce a defs_out (which is the final write to the symbol in a region) and a list of prior writes.

We don't actually realistically care about the last write for a single basic block, but realistically we care about the first write, so I need to check the code is following this correctly and doing the right thing.

[LonelyCat124]

ok - there is an issue with how i'm handling writes to the reference that I need to fix. I produce a defs_out (which is the final write to the symbol in a region) and a list of prior writes.

We don't actually realistically care about the last write for a single basic block, but realistically we care about the first write, so I need to check the code is following this correctly and doing the right thing.

This is fixed for general case with no control flow. I need to check this is fixed with control flow and calls/writes/codeblocks.

[LonelyCat124]

I've now handled CYCLE, BREAK and RETURN i think - let me know if you agree. Should be ready for another look if coverage is ok.

[LonelyCat124]

@sergisiso ready for another look I think.