(Towards #3367) def use chain update for assignments. by LonelyCat124 · Pull Request #3399 · stfc/PSyclone

LonelyCat124 · 2026-04-09T13:40:42Z

I now have this at a stage that it can handle multiple inputs and behave somewhat sensibly. I haven't yet finished, as it currently requires inputs to be members of the same parent, but I think we want them to simply share an ancestor statement.

There are a few remaining FIXMEs for things that I don't know what they were preventing previously and didn't cause any previous test failures, but presumably were things I thought mattered previously (though I did also just find some code that didn't matter so...). I need to do more testing for multi-inputs, but I think I might do that through Statement.next_accesses or some similar option.

codecov · 2026-04-09T14:26:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (f2023c8) to head (ac8fb18).

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #3399    +/-   ##
========================================
  Coverage   99.96%   99.96%            
========================================
  Files         391      391            
  Lines       54583    54689   +106     
========================================
+ Hits        54564    54670   +106     
  Misses         19       19

☔ View full report in Codecov by Sentry.
📢 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.

LonelyCat124 · 2026-04-10T15:08:27Z

I resolved the last of the FIXMEs. I need to do some tests with loops and things where I check I get the same results for Assignment.next_accesses/previous_accesses that I would calling it for all of the References individually.

LonelyCat124 · 2026-04-13T13:53:40Z

Having an issue with this code:

subroutine test
    integer :: a, b
    logical :: x
    a = b
    if (x) then
        a = b + a
    else
        b = a * b
    end if
    a = 1
    b = 1
    end subroutine test

For the single reference case, it doesn't find b=1 for a forward check for the original b Reference, and I can't quite work out the issue. There seems to be something with the b = a * b accesses seemingly not belonging to a control flow node, so the DUCs are assuming it must always happens and ends the chain, which is incorrect. I'm trying to sort this out but its a bit confused (as far as I can work out it also doesn't happen for the a access which is super weird.

…t shows some easy cases for forward accesses

LonelyCat124 · 2026-04-13T14:15:28Z

Fixed the previous case, but immediately created another non-equivalent case where I do

a = b
b = 2
...

as the def use chains output for b when the full statement is input goes past the b = 2 statement incorrectly.

LonelyCat124 · 2026-04-13T15:12:52Z

I fixed the above case going forwards, so things are looking like they're getting there. I suspect there will be issues going backwards that I'll need to match as well.

LonelyCat124 · 2026-04-14T12:35:43Z

NEMO ITs failing in the async runs. I need to manually look at whats happening - I think barriers are now being missed, which if there's not a simple solution to it might need a full DUC rewrite 😢

arporter

Thanks Aidan, that is extremely complex code! I've had a look through most of it except the tests so far. You'll see I'm a bit confused about the correct way of determining the "start point" and "end point". Apart from that, it's just minor things.

arporter · 2026-04-17T15:31:42Z

-            self._start_point = self._reference_abs_pos
+            # Find the highest abs position, as all of these are
+            # contained in the same parent.
+            self._start_point = max(list(self._references_abs_pos.values()))


As in, we only start the search after the last of the provided references?

Yes, I felt like for a next_accesses on a statement such as b = a + a we wouldn't want to return the second reference to a. I guess in theory with how the chains are currently setup for an example like b = a + a + a you could provide a list with only b and the first and last a references in which case we'd miss the middle a, but I think that isn't a real case so I'm not worried.

That makes sense, thanks. Worth saying this (the first sentence) in a comment.

arporter · 2026-04-17T15:32:29Z

-            self._stop_point = self._reference_abs_pos
+            # Find the max abs position, as all of these are
+            # contained in the same parent.
+            self._stop_point = min(list(self._references_abs_pos.values()))


You use min but say "max" in the comment :-)

In fact, as with my comments on the start_point, is min correct?

Updated the comment - I think min is correct as discussed in the previous comment, but I could be persuaded either way.

Please add a comment as discussed on the previous comment.

LonelyCat124 · 2026-04-20T14:03:02Z

@arporter Review comments addressed I think. I've tried to explain my reasoning for using max or min, but happy to change it if you disagree with what the result should be from the DUC.

…into 3367_def_use_chain_update

LonelyCat124 · 2026-04-21T14:35:29Z

I guess to also note this only improves the behaviour for Assignments, and the issue says statements, so I'm going to change this to towards 3367 and update the title and fix that later.

arporter

Thanks very much Aidan. I've made it all the way through now and it just comments and tidying left to do :-)
(ITs were all green bar a signal 11 on the LFRic tests which I've just restarted.)

arporter · 2026-05-01T13:11:00Z

                        )
-                        control_flow_nodes.append(None)
+                        index = len(chains)
+                        control_flow_nodes.insert(index, None)


I seem to be generally confused here. You get the length of chains and use that as the point at which to insert into control_flow_nodes - I don't think I'd fully understood that last time. Please add a comment to explain - this could also go some way to investigating whether (future) refactoring would help.

arporter · 2026-05-01T13:12:16Z

                        )
-                        control_flow_nodes.append(None)
+                        index = len(chains)
+                        control_flow_nodes.insert(index, None)


I'm not advocating for a re-write of anything at this stage - or at least, not in this PR. You understand the code a lot better than I do so if you think it would simplify things then that would be great, otherwise, don't worry.

arporter · 2026-05-01T13:16:53Z

-            self._start_point = self._reference_abs_pos
+            # Find the highest abs position, as all of these are
+            # contained in the same parent.
+            self._start_point = max(list(self._references_abs_pos.values()))


That makes sense, thanks. Worth saying this (the first sentence) in a comment.

arporter · 2026-05-01T13:17:38Z

-            self._stop_point = self._reference_abs_pos
+            # Find the max abs position, as all of these are
+            # contained in the same parent.
+            self._stop_point = min(list(self._references_abs_pos.values()))


Please add a comment as discussed on the previous comment.

arporter · 2026-05-01T13:34:35Z

+    sig = a_3.get_signature_and_indices()[0]
    duc = DefinitionUseChain(
-        a_3, control_flow_region=[routine]
+        [a_3], control_flow_region=[routine]


You don't need to put a_3 in a list now.

arporter · 2026-05-01T14:11:12Z

+
+    psyir = fortran_reader.psyir_from_source(code)
+    references = psyir.walk(Reference)
+    next_accesses = references[4].next_accesses()


Please add some comments to explain which bits of the code are being examined and found.

arporter · 2026-05-05T12:18:18Z

+    '''Test the DUCs give the same results for multiple inputs as each of
+    the inputs individually.
+    '''
+


It's a small thing but you could reduce duplication in the fixture by doing:

code = f"subroutine test\n{code}\nend subroutine test"

arporter · 2026-05-05T12:19:22Z

+    '''Test the DUCs give the same results for multiple inputs as each
+    of the inputs individually.
+    '''
+    psyir = fortran_reader.psyir_from_source(code)


Same comment here.

arporter · 2026-05-05T12:20:25Z

+    assign = psyir.walk(Assignment)[-1]
+
+    all_refs = assign.walk(Reference)
+    lhs_ref = all_refs[0]  # First Reference is lhs a


Probably worth sanity-checking this in case of future changes, e.g. assert lhs_ref.symbol.name == "a" and same for rhs_ref.

arporter · 2026-05-05T12:21:06Z

+    assign = psyir.walk(Assignment)[0]
+
+    all_refs = assign.walk(Reference)
+    lhs_ref = all_refs[0]  # First Reference is lhs a


Pls add sanity checks that the refs are the ones you expect, e.g. assert lhs_ref.symbol.name == "a"

LonelyCat124 · 2026-05-05T14:10:29Z

@arporter Ready for another look.

LonelyCat124 added 6 commits March 18, 2026 14:00

Some starting to def use chains update

166c9b7

[skip-ci] more duc improvements

23ddd68

[skip-ci] linting

e065541

[skip-ci] More improvements to DUC to enable multi-reference usage

eddd698

forward accesses first draft completed

fdcd6ad

Allow DUCs to handle multiple input References

010811a

LonelyCat124 requested review from arporter and sergisiso April 9, 2026 13:40

LonelyCat124 added 2 commits April 9, 2026 14:40

Merge branch 'master' into 3367_def_use_chain_update

120890f

fix tutorial

d12e7db

LonelyCat124 added the in progress label Apr 9, 2026

LonelyCat124 added 7 commits April 9, 2026 15:42

Remove unnecessary uniqueness check

fbdab62

Remaining coverage misses

576210d

allow all nodes in one statement

3f9da44

Update tests and add assignment.previous/next_accesses

63ef6b2

Fixed bug in ancestor assignment for searching backwards

4e72cac

Fix remaining FIXME

0194db8

Merge branch 'master' into 3367_def_use_chain_update

bfcda43

First identity test for DUC versions. This is still in development bu…

7677af0

…t shows some easy cases for forward accesses

Fixed forward accesses issue. Still need to check it going backwards

0fbe5f9

Fixed backward accesses and tests added

b84bba6

LonelyCat124 temporarily deployed to integration April 14, 2026 11:35 — with GitHub Actions Inactive

fixed issue with ITs

b566193

LonelyCat124 temporarily deployed to integration April 14, 2026 12:17 — with GitHub Actions Inactive

arporter requested changes Apr 17, 2026

View reviewed changes

arporter added reviewed with actions and removed under review labels Apr 17, 2026

LonelyCat124 added 3 commits April 20, 2026 13:40

Merge branch 'master' into 3367_def_use_chain_update

b80b529

Changes for review

533fe1c

Merge branch 'master' into 3367_def_use_chain_update

b5e5088

LonelyCat124 added 2 commits April 20, 2026 15:03

linting

dc5901f

Merge branch '3367_def_use_chain_update' of github.com:stfc/PSyclone …

4e9939d

…into 3367_def_use_chain_update

LonelyCat124 temporarily deployed to integration April 20, 2026 14:44 — with GitHub Actions Inactive

Merge branch 'master' into 3367_def_use_chain_update

32f950c

LonelyCat124 changed the title ~~(Closes #3367) def use chain update for statements.~~ (Towards #3367) def use chain update for assignments. Apr 21, 2026

LonelyCat124 added ready for review and removed reviewed with actions labels Apr 23, 2026

LonelyCat124 requested a review from arporter April 23, 2026 09:03

arporter added under review and removed ready for review labels May 1, 2026

Merge branch 'master' into 3367_def_use_chain_update

12de6a7

arporter temporarily deployed to integration May 1, 2026 13:22 — with GitHub Actions Inactive

arporter requested changes May 5, 2026

View reviewed changes

arporter added reviewed with actions and removed under review labels May 5, 2026

Changes from review

ac8fb18

LonelyCat124 requested a review from arporter May 5, 2026 14:08

LonelyCat124 added ready for review and removed reviewed with actions labels May 5, 2026

Conversation

LonelyCat124 commented Apr 9, 2026

Uh oh!

codecov Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LonelyCat124 commented Apr 10, 2026

Uh oh!

LonelyCat124 commented Apr 13, 2026

Uh oh!

LonelyCat124 commented Apr 13, 2026

Uh oh!

LonelyCat124 commented Apr 13, 2026

Uh oh!

LonelyCat124 commented Apr 14, 2026

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LonelyCat124 commented Apr 20, 2026

Uh oh!

LonelyCat124 commented Apr 21, 2026

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov Bot commented Apr 9, 2026 •

edited

Loading