(Closes #3367) statement next_accesses

[LonelyCat124]

No description provided.

[LonelyCat124]

I've not implemented the tests for this yet - I'm not sure if there should be any specific order to the result (or whether that should matter). I'm wondering if it should be sorted by abs_position or not.

I do think that for some blocks the way this has to be implemented at current will be prohibitively expensive, but the only solution to that will be a rewrite of the DUCs.

However, for the LFRic halo exchange infrastructure that requires this code I'm hopeful its not too expensive.

[LonelyCat124]

Looking at #3399, the way I implemented this for Assignments was to return a dictionary of Signature, List[Node] (so multiple lists for each variable in the Assignment).

In this PR I thought that distinguishing between them is not so relevant, the caller should be worrying about the result, and next/previous_accesses should always return just list[Node] to be consistent regardless of node type (where supported).

@arporter @sergisiso Do you agree? If so I'll modify Assignment before I continue with this implementation (as its currently stuck in testing due to this).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (72527a7) to head (fc7456b).

Additional details and impacted files

@@            Coverage Diff             @@
##            master     #3560    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          397       398     +1     
  Lines        55775     55944   +169     
==========================================
+ Hits         55775     55944   +169     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sergisiso]

It would be good to have a reminder/plan about how we are going to use this to do the HEx with the DUC.

Does completing #3124 changes the need for these? For instance in the loop implementation you have self.variable_reference.next_accesses() ... but this should not be necessary anymore now that this is a children?

Are this methods equivalent to:
accesses = {ref.next_accesses() for ref in self.walk(Reference)}
?

[LonelyCat124]

It would be good to have a reminder/plan about how we are going to use this to do the HEx with the DUC.

Does completing #3124 changes the need for these? For instance in the loop implementation you have self.variable_reference.next_accesses() ... but this should not be necessary anymore now that this is a children?

I don't remember to be honest, I think we need Kern to be a Call and then it inherits next_access and should "just work" to reimplement forward_dependence and backward_depedence (that I understood are the functions used by the HEx?) to use this functionality.

I could just do for child in loop.children probably, I currently didn't consider the start/stop/step which is probably wrong, so I should fix that up.

Are this methods equivalent to: accesses = {ref.next_accesses() for ref in self.walk(Reference)} ?

Not quite, it should be similar, but with node equality rules that wouldn't quite work I think. It also doesn't return any accesses contained inside the Statement itself, which for Loops would occur for accessing those references individually.

Swapping Assignment to just return a singular list was easy and didn't cause any side effects anywhere that I can see (as it was done in preperation for this PR I believe) so I've made that change and will continue to implement tests, as I think they'll make it clearer.

[sergisiso]

Not quite, it should be similar, but with node equality rules that wouldn't quite work I think. It also doesn't return any accesses contained inside the Statement itself, which for Loops would occur for accessing those references individually.

I'm a bit lost by this, could you show an example of an statement where it would have different results?

[LonelyCat124]

Not quite, it should be similar, but with node equality rules that wouldn't quite work I think. It also doesn't return any accesses contained inside the Statement itself, which for Loops would occur for accessing those references individually.

I'm a bit lost by this, could you show an example of an statement where it would have different results?

do i = 1, 100
   j = 4
end do
j = 3

If you just do psyir.walk(Assignment)[0].next_accesses() you'll get a self-reference to j=4 and then j=3. The loop.next_accesses() will just give the reference in j=3.

[sergisiso]

I don't remember to be honest, I think we need Kern to be a Call and then it inherits next_access and should "just work" to reimplement forward_dependence and backward_depedence (that I understood are the functions used by the HEx?) to use this functionality.

My worry is that next_accesses would return the ReadAfterRead without a way to differenciate them? In the Reference.next_accesses it is not a problem because we can do the is_read/is_write of both the location and target. But in a next_accesses of the statement we cannot do that.

If you just do psyir.walk(Assignment)[0].next_accesses() you'll get a self-reference to j=4 and then j=3. The loop.next_accesses() will just give the reference in j=3.

Then maybe something like:

accesses = {ref.next_accesses() for ref in self.walk(Reference)}
accesses = filter(lambda x: not x.is_descendant_of(self), accesses)

[LonelyCat124]

I don't remember to be honest, I think we need Kern to be a Call and then it inherits next_access and should "just work" to reimplement forward_dependence and backward_depedence (that I understood are the functions used by the HEx?) to use this functionality.

My worry is that next_accesses would return the ReadAfterRead without a way to differenciate them? In the Reference.next_accesses it is not a problem because we can do the is_read/is_write of both the location and target. But in a next_accesses of the statement we cannot do that.

If you just do psyir.walk(Assignment)[0].next_accesses() you'll get a self-reference to j=4 and then j=3. The loop.next_accesses() will just give the reference in j=3.

I guess I don't remember if we have some way to check if a Statement has read/write access to a variable with reference_accesses anyway? I think thats sufficient to distinguish between WaR and RaR.

Then maybe something like:

accesses = {ref.next_accesses() for ref in self.walk(Reference)}
accesses = filter(lambda x: not x.is_descendant_of(self), accesses)

Because nodes aren't hashable this doesn't work, we can't put them in sets.

[LonelyCat124]

Also calling ref.next_accesses() repeatedly is currently inefficient (until the DUC rewrite) as assignment.next_accesses() is more efficient where possible, and I can probably do a similar trick for some of loop's nodes later.

[LonelyCat124]

Been a bit slow with this sorry, a first implementation is ready for a first look @sergisiso @arporter . I do (at some point during the review process) want to look at potentially improving the performance of the loop implementation, and maybe look at some of the DSL nodes implementations (may need some advice there), but a first review to see how it works would be better first I think.