(towards #3124) Make the loop variable as child of Loop instead of an attribute#3473
(towards #3124) Make the loop variable as child of Loop instead of an attribute#3473sergisiso wants to merge 10 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3473 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 395 395
Lines 55199 55212 +13
=========================================
+ Hits 55199 55212 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sergisiso
requested review from
LonelyCat124 and
arporter
and removed request for
arporter
|
@arporter @LonelyCat124 This is ready for review. Switching to in-tree loop references has been relatively painless, with the only oddity being the lfric null loops (which are loops that are not loops?) and have a dummy symbol as they don't exist after lowering anyway. |
There was a problem hiding this comment.
I had a very quick scan through and it looks to make sense. There's a leftover print statement and this opens up one other issue that wasn't previously addressed in the definition use chains.
Can you create a test using loop.variable.next_accesses() and check what the behaviour is? Once we know that I'll have a more detailed look through.
| assert isinstance(dag, graphviz.Digraph) | ||
|
|
||
| result = my_file.read() | ||
| print(result) |
@LonelyCat124 Is this what you wanted to check or do you mean a test with |
@sergisiso I guess a test with |
|
@LonelyCat124 I added the suggested check in references_test.py. Ready for another look |
LonelyCat124
left a comment
LonelyCat124
left a comment
There was a problem hiding this comment.
@sergisiso A few more changes
I think a variable_reference (or some better name) getter (rather than children[0]) is probably worthwhile to avoid random potential accesses to children[0].
Also I think Reference.is_write should be updated for these new References also?
| if self.variable.name != "null": | ||
| var_accesses.add_access(Signature(self.variable.name), | ||
| AccessType.WRITE, self) | ||
| # This re |
There was a problem hiding this comment.
This comment is unfinished.
| - the second one represents the loop lower bound, | ||
| - the third one represents the loop upper bound, | ||
| - the forth one represents the step value | ||
| - and the fith one is always a PSyIR Schedule node containing the |
| if len(self.children) < 4: | ||
| if len(self.children) < 5: | ||
| raise InternalError( | ||
| f"Loop is incomplete. It should have exactly 4 " |
There was a problem hiding this comment.
Error message is out of date.
| assert b.next_accesses()[0] == b | ||
|
|
||
| # Check the next_access of the loop variable | ||
| assert loop.children[0].next_accesses() == [loop.loop_body.children[0].rhs] |
There was a problem hiding this comment.
Thanks for this - can you add a test what happens if we have:
do i = 1, 100
a(1) = a(1) + 1
end do
I'm not sure what will happen with the next_access for i and want to make sure it makes sense and we understand it.
Also in definition_use_chains_forward_dependence_test.py line 664 we have a test that returns a Loop (instead of its variable). Perhaps the best thing to do is to create an issue "Definition use chains return a Loop instead of the Loops' variable reference" (or similar) and we add a TODO to all the tests that return these loops (and maybe a note in the DUC docs with the TODO also?) since there is now a reference that could be pointed at.
2 participants
No description provided.