-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compiled backends integration #835
Conversation
62b1f1f
to
da31c54
Compare
4ac41c3
to
95b1302
Compare
95b1302
to
1307bad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played a bit with this PR and have some suggestions. I also found the place that breaks the shift (offset_provider) is not forwarded (and not checked that it is there)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a preliminar review. In general it looks good, but there are some points which I think need to be improved before merging. Some of them might look like cosmetic issues which can be added later but in my experience, if some basic foundation is not added at creation time, it's highly unlikely that they get added later (and it'd be much harder to do anyway):
- Add standard headers to all the new files
- Add a brief docstring to the modules and the main classes / functions
- Add an ADR explaining the basic design of the toolchain and the pieces that need further work in the future.
- Add some try / except blocks in the places most likely to fail with a meaningful error message. It doesn't need to be everywhere or superpolished message, but just something.
Regarding the folder structure, I would suggest to rename fencil_processors/callables
to fencil_processors/common
or fencil_processors/tools
and move defs.py
(why not definitions.py
?) and cpp.py
inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks basically ready to go, the main issues have been solved. Just a couple of typos and some minor comments and optional suggestions.
docs/functional/architecture/0009-Compiled-Backend-Integration.md
Outdated
Show resolved
Hide resolved
docs/functional/architecture/0009-Compiled-Backend-Integration.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, thanks for the hard work! Just delete the unnecessary __init__.py
in source_module_tests
and it's good to go.
Description
Introduces an organized way to take source code generated from iterator IR, generate Python bindings for it, compile the source and the bindings into a Python module, and load it back into Python.
Operation:
Test parametrization:
Integration tests are parametrized to run with multiple backends, including compiled and immediate.
Known issues:
TODO:
Requirements
Before submitting this PR, please make sure:
Additionally, if this PR contains code authored by new contributors:
signed by the employer if needed, provided to ETH Zurich
version of the AUTHORS.rst file included in the PR