(closes #1010, #1910, #719, #798, #1648) Generate PSy-layers and Kernel Subs using PSyIR lowering (instead of gen_code/f2pygen) #2834
Conversation
…10_remaining_lfric_lowering
…rsor on invokes declarations and initialisations
… invoke comments
…nto 1010_remaining_lfric_lowering
…ith psyir backend
|
@TeranIvy @hiker I merged the latest changes to this branch (split elements order and partially written extraction field), and is now passing all our compilation and integration tests. It would be great if you could check it with the LFRic tests (@TeranIvy ) and additional extraction test (@hiker ) and let me know if there is any problem that was not reported by our CI.
|
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
Hi @sergisiso, I installed the PSyclone test environment from your branch and ran comprehensive Rose stem tests (
The failures in LFRic Atmosphere, Coupled and UM Physics Interface seem to be failures to find/create a symbol (error report below). Failure in NG Arch job is similar to the above ones (same variable not in symbol table but manifests in another algorithm call, see below. The failures in LFRic JEDI and Adjoint tests builds return a different error which is not so obvious to debug. GungHo model failure is due to declarations of unused variables (seems to be MO issue). |
|
The issue with gungho is that lfric_apps uses a different script just for algorithm/transport/ffsl/ffsl_advective_updates_alg_mod.py |
|
Retested LFRic Core and Apps with PSyclone test environments. This time I created branches for testing, and merged Josh's changes for move to PSyclone 3.1.0 to the Apps branch to prevent PSyAD failures. The branches are below (up to the respective trunks this morning). Core tests are all fine. However, there are still failures in Apps tests. Some JEDI and Adjoint tests throw errors that are not obvious but seem to go back to the certain algorithms: and and |
| else: | ||
| # If it has a tag, first try to look up for it | ||
| try: | ||
| sym = self._symtab.lookup_with_tag("tag") |
There was a problem hiding this comment.
Probably you meant tag without the quotes? That might then fix the coverage issue.
There was a problem hiding this comment.
Oh yes, that's embarrassing, that also fixes the first issue reported by @TeranIvy
| "call.*" | ||
| "end.*" | ||
| r"CALL (?P=profile2) % PostEnd") | ||
| code = fortran_writer(invoke.schedule) |
There was a problem hiding this comment.
The comment here needs updating now we don't remove all new lines or use a regex.
| # with '-Werror=unused-variable' | ||
| routine = node1.ancestor(Routine) | ||
| if routine: | ||
| remaining_syms = [r.symbol for r in routine.walk(Reference)] |
There was a problem hiding this comment.
I'm unsure whether it would be better here to use:
vai = VariablesAccessInfo(routine)
all_names = [sig.var_name for sig in vai.all_signatures]
There was a problem hiding this comment.
I am slightly nervous that Signatures still operate on strings and can't differentiate between symbols with the same name in different scopes, but I think is the right thing to do here, so I updated to it.
| if routine: | ||
| remaining_syms = [r.symbol for r in routine.walk(Reference)] | ||
| del_syms = [r.symbol for r in node2.start_expr.walk(Reference) + | ||
| node2.stop_expr.walk(Reference)] |
There was a problem hiding this comment.
Strictly speaking, we should allow for step too.
| @@ -198,6 +198,21 @@ def apply(self, node1, node2, options=None): | |||
| # Add loop contents of node2 to node1 | |||
| node1.loop_body.children.extend(node2.loop_body.pop_all_children()) | |||
|
|
|||
| # We need to remove all leftover references because lfric is compiled | |||
| # with '-Werror=unused-variable' | |||
There was a problem hiding this comment.
Could add: ". Since we have fused loops, we only need to look at the symbols appearing in the loop control of the second loop."
| @@ -38,7 +38,8 @@ | |||
| from psyclone.configuration import Config | |||
| from psyclone.psyir.nodes import ( | |||
| OMPDoDirective, OMPLoopDirective, OMPParallelDoDirective, | |||
| OMPTeamsDistributeParallelDoDirective, OMPScheduleClause) | |||
| OMPTeamsLoopDirective, OMPTeamsDistributeParallelDoDirective, | |||
| """diff_basis_w2_qr) | ||
| CALL extract_psy_data%PreDeclareVariable("e", e) | ||
| CALL extract_psy_data%PreDeclareVariable("istp", istp) | ||
| CALL extract_psy_data%PreDeclareVariable("last_edge_cell_all_colours", \ |
There was a problem hiding this comment.
Aside from the minor matter that various LFRic rose-stem tests are failing, I think this is the last remaining thing to resolve. @hiker have you had time to see how much work it is likely to be?
|
@TeranIvy Can you try again. I have been able to run them locally and now this are fixed: "> Some ngarch, lfric_atm, lfric_coupled and um_physics tests fail with Symbol errors", The adjoint and jedi failures are when applying |
@sergisiso, thanks for fixig the issues! I retested both Core and Apps branches after bringing them up to heads of respective trunks.
I think this is fine from our side. |
|
@arporter I addressed your latest comments, fixed the rose-stem test issues (I have all* lfric apps running locally now I will add these to the integration tests but in a different PR), brought to master and improved the coverage. This is ready for another review. *all apps but the adjoint patches. @arporter @TeranIvy Relying on the exact output of psyclone will always be very brittle, I don't think we should be doing backwards-compatibility guarantees on this but the number of patches in that folder is big. I know there is a PSyAD issues github project, but do we know if there is a set of issues that cause the need for most of the patches? |
This is a question for @DrTVockerodtMO really. |
No description provided.