3111 remove loop var from extraction #3152
Conversation
…r_from_extraction
…var_from_extraction
hiker
added
enhancement
Blocked
…r_from_extraction
…var_from_extraction
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3152 +/- ##
=======================================
Coverage 99.90% 99.90%
=======================================
Files 376 376
Lines 52952 53041 +89
=======================================
+ Hits 52900 52989 +89
Misses 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…var_from_extraction
…r_from_extraction
hiker
added
ready for review
and removed
Blocked
|
Ready for review. I did trigger the CI 'recently' (yesterday or Sunday?). Note that this only replaces loop variable. I will do a follow up PR to also remove the temporary array used when reproducible reductions are enabled. |
| ''' | ||
| def __init__( | ||
| self, access_type: AccessType, node: 'Node', | ||
| self, access_type: AccessType, node: Union['Node', Symbol], |
There was a problem hiding this comment.
Just for my understanding, does 'Node' need to be in quotes here because it's only imported if TYPE_CHECKING is true?
|
|
||
| @property | ||
| def all_read_accesses(self): | ||
| def all_read_accesses(self) -> list[str]: |
There was a problem hiding this comment.
Should be list[AccessInfo]? Also, please rm the the rtype from the docstring.
| a different signature. | ||
| ''' | ||
| if self._signature != access_seq.signature: | ||
| raise ValueError(f"Updating AccessSequence for '{self.signature}' " |
There was a problem hiding this comment.
How about "Cannot update the AccessSequence for '{self.signature}' using data for a different access ('{access_seq.signature}')."
| signature: Signature, | ||
| access_type: AccessType, | ||
| node: "Node", | ||
| component_indices: Optional[ComponentIndices] = None) -> None: |
There was a problem hiding this comment.
I've just realised this is going to clash with #3164 (which removes ComponentIndices in favour of dynamic calculation).
| postfix: str, | ||
| removable_vars: list[tuple[str, Signature]], | ||
| ) -> list[tuple[Symbol, Symbol]]: | ||
| '''This function creates the code that reads in the data file |
There was a problem hiding this comment.
Please document the new removable_vars argument.
There was a problem hiding this comment.
Also, I think vars_to_ignore would be a better name - I was expect the code to actually have to remove things but it doesn't - it just ignores them.
| # discarded (e.g. a loop variable might be declared as omp private). | ||
| # We do this by counting how many directive nodes are in the list, and | ||
| # then subtracting this number from all accesses: | ||
| for var_sig, accesses in all_loops.items(): |
There was a problem hiding this comment.
I did wonder whether it might be better to use the new, Reference.escapes_scope() functionality but a) we don't have a Reference for a loop variable (we'd have to find one) and b) this would require extra care if a loop variable was re-used as the variable for another loop. All in all, I think this approach is good.
| ctu = CallTreeUtils() | ||
| read_write_info = ctu.get_in_out_parameters( | ||
| self, include_non_data_accesses=False) | ||
| removable_vars = self.get_removable_variables() |
There was a problem hiding this comment.
Again, I think "vars_to_ignore".
| region_name=region_name_tuple, | ||
| removable_vars=removable_vars) | ||
|
|
||
| for var_info in removable_vars: |
There was a problem hiding this comment.
Comment please - "remove the variables that we're going to ignore"
| ''' | ||
| suffix = "" | ||
| suffix: Union[str, int] = "" |
There was a problem hiding this comment.
Since you then have to use a cast below, I suggest we leave this type hinting for now.
| ''' | ||
| already_flattened = {} # dict of name: symbol | ||
| already_flattened: dict[str, Symbol] = {} # dict of name: symbol |
There was a problem hiding this comment.
This too is going to clash quite badly with #3164.
This needs #2049 / #3131 to be merged in. It has a lot of clashes with 2049, so I've merged in 2049 here and resolved them.