(closes #2823) Parse unresolved symbols with parenthesis as calls #3041
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3041 +/- ##
=======================================
Coverage 99.90% 99.90%
=======================================
Files 376 376
Lines 53157 53213 +56
=======================================
+ Hits 53105 53161 +56
Misses 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…with_usage_location method
…ormance for NEMOv4
…s (and not offloading much)
|
@arporter This is ready for another review. Some comments are outdated due to preceding PRs. |
There was a problem hiding this comment.
Looks good Sergi, thanks very much. I got a bit worried about the fact that we might be breaking PSyAD but, when I tried, it seemed fine. I agree that the frontend is the right place to do the reasoning about the type of unresolved symbols and I like that change. As discussed, I think we can safely extend it to say that a symbol that is accessed with and without parentheses must be an array.
src/psyclone/tests/domain/common/transformations/raise_psyir_2_alg_trans_test.py
Show resolved
Hide resolved
|
@arporter This is ready for another review. I addressed all comments but note that the integration test showed a small difference in the nemo async workflow. I will explore this more to see if it is replicable and what file it comes from in the meantime. |
There was a problem hiding this comment.
Looking good now Sergi, thanks very much. Well done on the fixes to preserve the Sympy behaviour.
Just a bit more tidying to do (and wrangling over whether we raise an InternalError :-) ).
I wondered whether the documentation mentioned the fact that we (used to) assume a part reference was an array access but I couldn't find anything.
| shape = node.symbol.shape | ||
| else: | ||
| # If we don't know the dimension we make it look like a | ||
| # function call without argumetns, this will make it not |
|
|
||
| return (f"{name}{self.array_parenthesis[0]}" | ||
| f"{','.join(result)}{self.array_parenthesis[1]}") | ||
| if name in self.type_map: |
| symbol_type=DataSymbol, | ||
| datatype=UnresolvedType()) | ||
| if name in direct_refnames_in_exprs: | ||
| # If this any other expression has the same reference name without |
| if name in direct_refnames_in_exprs: | ||
| # If this any other expression has the same reference name without | ||
| # parenthesis, e.g.: a + a(3), we know a is an array and not a | ||
| # function call, as the later have mandatory parenthesis. |
| array_assignment = schedule.children[1] | ||
| assert isinstance(array_assignment, Assignment) | ||
| var_accesses = array_assignment.reference_accesses() | ||
| # We don't know if 'f' is a function or an array (CALLED or READ), so |
| module = parse_tree.children[0] | ||
|
|
||
| # By default this will all be parsed as Calls with unknown | ||
| # is_lememental/is_pre attributes |
|
|
||
| assert loop.start_expr.datatype == INTEGER_TYPE | ||
| assert loop.stop_expr.datatype == INTEGER_TYPE | ||
| assert loop.step_expr.datatype == INTEGER_TYPE |
There was a problem hiding this comment.
This one is still outstanding. Possibly your new 'refine_symbols_with_usagemethod in the frontend could be extended for this. Similarly for precision symbols (e.g 1.0_wp meanswp` is an integer).
| f"'{self.debug_string()}' include a function call or " | ||
| f"unsupported feature. Querying the return type of " | ||
| f"such things is yet to be implemented.") | ||
| # The validate only allows [Range | DataNode] children, so there is |
There was a problem hiding this comment.
Oh yes, I can see that it was this way before. However, I think it's useful to defend against these cases as it saves time later - if we come to modify e.g. _validate_child in future and forget to modify this routine at the same time it could cause some pain.
…is need to be reverted)
3 participants
No description provided.