(Towards #1823) Permit only module-inlined Kernels to be transformed#3294
(Towards #1823) Permit only module-inlined Kernels to be transformed#3294
Conversation
|
Possibly I need to store the RoutineSymbol associated with a Kernel within the Kernel class (c.f. Call.reference). That way, I can tell whether it has been module-inlined or not and can get rid of the special flag for that. |
|
Note to self: I will need to remove the EDIT: can't do this because we do still output OpenCL versions of kernels in some tests. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3294 +/- ##
==========================================
- Coverage 99.95% 99.95% -0.01%
==========================================
Files 381 382 +1
Lines 53995 53878 -117
==========================================
- Hits 53973 53856 -117
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Coverage is now all good but the LFRic extraction integration test failed :-( |
|
In lfric/eg17/full_example_extract: $ diff main_psy.f90 on_main/main_psy.f90
4d3
< use testkern_w0_kernel_mod, only : testkern_w0_code
74a74
> use testkern_w0_kernel_mod, only : some_other_var, testkern_w0_codeso it appears the |
|
This turned out to be due to the fact that I've moved the import of Kernel routines up into the Container and then the ExtractNode was falling foul of #1734 |
|
Assuming the ITs come back green this time, this is ready for review. Primarily, it adds checks such that any kernel transformations will fail unless the kernel has already been module inlined. Unfortunately, updating some of those transformations got a bit hairy (e.g. to deal with multiple calls to the same kernel). The situation will improve again once we re-name module-inlined routines (which this PR is a pre-cursor to). As part of this, I moved the location of the USE statements for kernel modules up to the container level. This affected the text checked for in quite a few tests. |
sergisiso
left a comment
sergisiso
left a comment
There was a problem hiding this comment.
@arporter I am really happy to see that we are getting rid of rename_and_write.
This is an important change with implications to the CLI and output behaviour. I would prefer that you create an issue for it instead of being just a towards a meta-issue with many other things.
Now that I write that, this probably means the user guide needs to be updated and I'm pretty sure I haven't done that.
This. But also looking at the documentation, can we delete the "--kernel-renaming" option from generator.py?
Now that we require a kernel to be module-inlined before it is transformed,
Not necessarily, LFRic can now ask psyclone to process kernel files (it is essentially what 'transmute' is). It is more "now we require kernels specifically modified for an invoke to be module-inlined first". I don't know if it belongs to our documentation, but if we do we want to be clear about it.
| # Convert any accesses to imported data into kernel arguments, put an | ||
| # 'acc routine' directive inside, and module-inline each kernel | ||
| for kern in schedule.coded_kernels(): | ||
| itrans.apply(kern) |
There was a problem hiding this comment.
Could you rename it to mod_inline_trans (to prevent confusions with i(nline_)trans)
| f"because") | ||
|
|
||
| rsymbol = node.scope.symbol_table.lookup(node.name, otherwise=None) | ||
| if not rsymbol or rsymbol.is_import or rsymbol.is_unresolved: |
There was a problem hiding this comment.
maybe: if not rsymbol or not rsymbol.is_modulevar: ?
| assert ("Cannot transform this Kernel call to 'kernel_with_global_code' " | ||
| "because it is not module inlined" in str(err.value)) |
There was a problem hiding this comment.
I am wondering if we shouldn't assume that users know what "module inline" means here, as we use it but it has been confusing before. Maybe the error could be:
"Cannot transform this kernel because its implementation resides in a different file, apply the KernelModuleInlineTrans first to bring it to this module".
There was a problem hiding this comment.
Oh looking at KernelTransformationMixin message it is different. Do we double check this condition?
| assert expected_output in out | ||
|
|
||
|
|
||
| def test_codedkern_module_inline_getter_and_setter(): |
There was a problem hiding this comment.
The getter still exists, do we want to keep a test for it?
2 participants
No description provided.