2704 backward accesses #2814
2704 backward accesses #2814
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2814 +/- ##
========================================
Coverage 99.88% 99.88%
========================================
Files 357 357
Lines 49724 49876 +152
========================================
+ Hits 49668 49820 +152
Misses 56 56 ☔ View full report in Codecov by Sentry. |
…they weren't inside the search area, and added tests for previous_accesses in Reference class
|
@sergisiso This is ready for a first look now I think |
There was a problem hiding this comment.
@LonelyCat124 This is in a good shape, all the comments are minor. Some of the logic is quite complicated but since this is self-contained and there are a good amount of tests I am happy with it.
| if ref is self._reference: | ||
| in_else_body = True | ||
| break | ||
| # If its in the else_body we don't add the if_body |
There was a problem hiding this comment.
Maybe indent this comment as the "break" (and maybe put it before the break)
| "DefinitionUseChains can't " | ||
| "handle code containing GOTO" | ||
| " statements." |
There was a problem hiding this comment.
This can be condensed to two lines with this indentation
| f"must be a Reference but found " | ||
| f"'{type(reference).__name__}'.") | ||
| raise TypeError( | ||
| f"The reference passed into a DefinitionUseChain " |
There was a problem hiding this comment.
This is from the previous PR, but "The 'reference' argument passed ..." will be clearer
| "DefinitionUseChains can't " | ||
| "handle code containing GOTO" | ||
| " statements." |
| @@ -333,7 +339,12 @@ def find_forward_accesses(self): | |||
| # If the control flow node is a Loop we have to check | |||
| # if the variable is the same symbol as the _reference. | |||
| if isinstance(cfn, Loop): | |||
| if cfn.variable == self._reference.symbol: | |||
| cfn_abs_pos = cfn.abs_position | |||
There was a problem hiding this comment.
From the previous PR, but in the comment above s/could not be taken/could be 'not taken'/
| # This doesn't work correctly if the Reference that | ||
| # is having its backwards dependencies analysed occurs after | ||
| # one of these such statements in a basic block, however | ||
| # since they're unreachable maybe we don't care? |
There was a problem hiding this comment.
I agree, this is more evidence that we need a DeadCodeEliminationTrans to run early in the process so we get rid of all this.
There was a problem hiding this comment.
I guess is this a transformation we'd want to run in the frontend? Or is there a good reason to leave dead code?
| self._stop_point = save_stop_position | ||
| return self._reaches | ||
| else: | ||
| # We assume that the control flow here could not be taken, |
There was a problem hiding this comment.
Again, maybe its just me but "could not be taken" seems that something when wrong, so maybe "could be 'not taken'" to make clear that we talk about the chain branches?
There was a problem hiding this comment.
Yeah, changed, that makes more sense.
| # TODO Need to fix this - not sure what is going on here. | ||
| # If we get here to check the start part of a loop we need | ||
| # to handle this differently. |
There was a problem hiding this comment.
Does this still need fixing?
There was a problem hiding this comment.
Nope, removed the comment.
| elif reference.get_signature_and_indices()[0] == sig: | ||
| # Work out if its read only or not. | ||
| assign = reference.ancestor(Assignment) | ||
| # TODO Need to invert how we think about assignments. |
There was a problem hiding this comment.
Is this TODO still necessary?
There was a problem hiding this comment.
Nope, this works now - removed the comment.
| end subroutine''' | ||
| psyir = fortran_reader.psyir_from_source(code) | ||
| routine = psyir.children[0] | ||
| a = routine.children[1].lhs |
There was a problem hiding this comment.
For clarity maybe call it "a_after_loop" and a_2 call it "a_inside_loop"
There was a problem hiding this comment.
Actually this a_2 was never used so I've now removed it.
Bugs fixed, initial implementation - need to add some other parts to it.