search

microsoft / knossos-ksc

Compiler with automatic differentiation
Other
45 stars 10 forks source link

Rename compilation functions #906

Closed dcrc2 closed 3 years ago

dcrc2 commented 3 years ago

The file compile.py has various functions for generating C++ code and compiling it, but I've been finding it quite difficult to remember which functions generate which parts of the code. This PR carries out various renamings which aim to make this a bit clearer.

dcrc2 commented 3 years ago

I think as it's a follow on of earlier deprecation, maybe merge onto the branch to make that easier to see?

It wasn't really meant that way, to be honest. Though doing it after the removal of the deprecated function meant that there was one fewer thing to rename.

toelli-msft commented 3 years ago

This changes things so that the cpp_ks_functions is emitted before the #include "knossos-pybind.h" line, which is probably fine. I wouldn't expect any change in behaviour to result from that.

I'm not sure why declarations_to_generate has been changed to entry_points in one place and bindings_to_generate in another place, despite them being seemingly very similar things, but I'm sure there's a good reason.

dcrc2 commented 3 years ago

This changes things so that the cpp_ks_functions is emitted before the #include "knossos-pybind.h" line, which is probably fine. I wouldn't expect any change in behaviour to result from that.

Yes, there shouldn't be any change. I wanted to avoid interspersing cpp_ks_functions with cpp_pybind_module_declaration, partly to make it clear that those things can be generated independently, but also because for a GPU version they will actually need to be in separate files (cpp_ks_functions in a .cu file and cpp_pybind_module_declaration in a .cpp file).

I'm not sure why declarations_to_generate has been changed to entry_points in one place and bindings_to_generate in another place, despite them being seemingly very similar things, but I'm sure there's a good reason.

I wanted to change the function generate_cpp_for_py_module_from_ks in particular, where we're using declarations_to_generate both for the python-to-ks mapping and the python-to-cpp mapping (which have different types). Then I applied the new names throughout ... but maybe that was going a bit far. I do quite like "bindings" rather than "declarations" though. So I've just pushed a change which uses bindings_to_generate everywhere, except for that one place in generate_cpp_for_py_module_from_ks.

toelli-msft commented 3 years ago

This changes things so that the cpp_ks_functions is emitted before the #include "knossos-pybind.h" line, which is probably fine. I wouldn't expect any change in behaviour to result from that.

Yes, there shouldn't be any change.

Amusingly, there was a change. My cpp_string_to_autograd_function embedded code had been using things defined in knossos.h which was implicitly included via the knossos-pybind.h include. Manually importing knossos.h resolves that problem.