cython icon indicating copy to clipboard operation
cython copied to clipboard

RFC: Adding a backend for HPy.

Open fangerer opened this issue 3 years ago • 15 comments

This PR is a first effort for the long-term goal to add an HPy backend to Cython. The main objective here is to get some feedback on how to structure the code and what else needs to be considered. So, this is an explicit request for comments.

The main idea is to move C code generation into the CCodeWriter. For that, I've introduced a new class HPyCodeWriter that inherits from CCodeWriter and the new writer emits HPy API calls/code instead of CPython API calls/code. Then, we can use two different writers to run the same generation step.

As we have already discussed in a meeting on September 3rd, 2021 with @scoder, Cython now emits C API and HPy API code and the different sections are protected by ifdefs like this:

# ifndef HPY
  /* C API code */
#endif

#ifdef HPY
  /* HPy API code */
#endif

This is realized by introducing code sections for HPy (see Code.py: code_layout). Some of the sections are independent of the API and can be shared (e.g. string_decls which defines global const char * variables).

A lot of work still needs to be done (of course) but the first simple example hpy_basic.py is already working (C only).

fangerer avatar Dec 06 '21 11:12 fangerer

Wow, thanks. That's … huge. :)

Do I understand correctly that this duplicates the C code, using two separate code writers? Or is the duplication limited to certain sections or constructs?

What I would like to avoid is that we end up with two separate copies of the user code in the same file. I was hoping that we could do as much as possible through C macros, without actually duplicating generated code.

scoder avatar Dec 06 '21 14:12 scoder

Do I understand correctly that this duplicates the C code, using two separate code writers? Or is the duplication limited to certain sections or constructs?

The duplication is limited to certain sections. The easiest for now was to introduce respective sections as specified in GlobalState.code_layout. The HPy-specific sections have prefix hpy_sectionname. If there is such a section, the hpy_sectionname is surrounded by #ifdef HPY ... #endif and the related section sectionname is surrounded by #ifndef HPY ... #endif.

I've tried to share any API independent code. For example, section string_decls is not duplicated.

This is, maybe, one of the main question: what should be the granularity of sharing. My opinion was that multiplexing code on a statement-level is too fine-grained. Also, it would in some cases just not work out because we need to generate a significantly different structure. One example would be Python types: While it is common to define and fill a static type struct in the C API, for HPy we will need to generate a type specification.

What I would like to avoid is that we end up with two separate copies of the user code in the same file. I was hoping that we could do as much as possible through C macros, without actually duplicating generated code.

I absolutely agree. I will do my best to share as much code as possible. Sometimes it's just a trade-off between Cython source code complexity and generate source code sharing.

fangerer avatar Dec 07 '21 10:12 fangerer

@scoder Thanks for your comments. My main question, of course, is: How should I proceed? Is it fine to move code generation into the code writers? Or should we have a separate utility for that?

It may also be interesting to look at the generated C code of the hpy_basic example (see attachment hpy_basic.c.gz ).

fangerer avatar Dec 07 '21 11:12 fangerer

I mostly agree with @da-woods regarding the code writer changes. Anything that is specific to the way a syntax node (or a certain kind of nodes) generates code should stay in the node classes. I'm not convinced yet that we need a second kind of code writer. If we can solve the issue with C macros, then we should.

* I'm not completely sure where we draw the distinction, but HPy feels more like a ["compiler option"](https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#compiler-options) than a compiler directive to me.

I agree. Also that the distinction isn't very clear. :)

This feature leads to a potentially large amount of additional code being generated, in which case users should enable it very explicitly. However, I do still hope that the amount won't be that large in the end. (Which reminds me that this huge bunch of defines from the module state struct code still needs to be cleaned out…)

  1. For every module we generate both HPy and CPython code, and select between them by passing a C compiler define. This matches what we currently do, and I think was the solution that @scoder was keen on
  2. Based on what the user requests, we generate _only_ of HPy or CPython code. I think this is closer to what you're doing.
  
  Personally, I was always more comfortable with option 2 - I thought that the two interfaces were sufficiently different that it'd be hard to do only with `#define`s.

I was thinking that Cython should generate more HPy-like code and map that back to "legacy" C-API code with C macros. My impression was that this approach might be easier than the other way, since HPy makes stricter assumptions. Also, since HPy code is supposed to work on CPython, that layer partly already exists. Although it probably needs to be tuned for Cython to avoid any regressions.

For example, I believe that HPy will distinguish between long-lived references (objects in a class or a generator) and short-lived references (objects in a function), which isn't a distinction that Cython currently makes.

Interesting. I wasn't aware that such a distinction was needed. Why is that?

scoder avatar Dec 12 '21 11:12 scoder

While it is common to define and fill a static type struct in the C API, for HPy we will need to generate a type specification.

We already generate type specs for CPython. HPy could either use those, adapt those as needed, or add one more (in the order of preference).

scoder avatar Dec 12 '21 12:12 scoder

For example, I believe that HPy will distinguish between long-lived references (objects in a class or a generator) and short-lived references (objects in a function), which isn't a distinction that Cython currently makes.

Interesting. I wasn't aware that such a distinction was needed. Why is that?

Quoting from https://docs.hpyproject.org/en/latest/overview.html:

handles are intended to be short-lived, but sometimes one needs a long-lived reference to a Python object. In HPy, we call this long-lived reference an HPyField, but we still need to implement it. We also need HPy_Store and HPy_Load to save and load these fields. This will allow alternative implementations to use a moving GC.

I don't think that's actually implemented yet though... (edit: yes it actually says that and I just didn't read thoroughly)

da-woods avatar Dec 12 '21 12:12 da-woods

Quoting from https://docs.hpyproject.org/en/latest/overview.html:

handles are intended to be short-lived, but sometimes one needs a long-lived reference to a Python object. In HPy, we call this long-lived reference an HPyField, but we still need to implement it. We also need HPy_Store and HPy_Load to save and load these fields. This will allow alternative implementations to use a moving GC.

I don't think that's actually implemented yet though... (edit: yes it actually says that and I just didn't read thoroughly)

Hmm. Is that a strict requirement, though? I'd rather get things working without this, and then adapt where we deem it helpful. Globals are probably ok, but I'm not sure yet about object struct references. Let's try not to optimise prematurely here.

scoder avatar Dec 12 '21 12:12 scoder

Hmm. Is that a strict requirement, though? I'd rather get things working without this, and then adapt where we deem it helpful. Globals are probably ok, but I'm not sure yet about object struct references. Let's try not to optimise prematurely here.

I think it's probably not a strict requirement given it's on the HPy plan but not yet implemented. I expect it'll reduce the performance of some GCs rather than break anything.

da-woods avatar Dec 12 '21 12:12 da-woods

Again, thanks for your feedback.

I mostly agree with @da-woods regarding the code writer changes.

Okay, I'll move that code back to the nodes. I'm currently experimenting with using more meta-programming do solve that problem. For example: I've introduced a macro __Pyx_OBJECT_CTYPE which is PyObject * for the C API and HPy for HPy.

I'm not completely sure where we draw the distinction, but HPy feels more like a "compiler option" than a compiler directive to me.

That's, ofc, fine for me. I'll go for the compiler option. If I understood correctly: Cython should only generate HPy code if this option is enabled, right?

I was thinking that Cython should generate more HPy-like code and map that back to "legacy" C-API code with C macros. My impression was that this approach might be easier than the other way, since HPy makes stricter assumptions.

Sounds good.

Concerning HPyField: Yes, it's already implemented but not included in any release yet. We plan to include it in HPy 0.0.4. My current (local) changes don't use it but I think I will adopt. The point is: In HPy, we do not guarantee that there is just one global HPy context and normal handles do only exist in the scope of an HPy context. The current HPy implementation for CPython just uses one context but that's subject to change (in particular for sub-interpreters or similar). So, storing objects into C globals must use HPyField. Also thanks for quoting from https://docs.hpyproject.org/en/latest/overview.html . I need to update it.

I'll incorporate your feedback and will push these changes ASAP.

fangerer avatar Dec 15 '21 14:12 fangerer

Cython should only generate HPy code if this option is enabled, right?

Yes. And it should be enabled by default, as soon as there is anything useful in the port.

EDIT: although the default is to be redecided once that's the case. :)

scoder avatar Dec 15 '21 15:12 scoder

Sorry that it took that long but I just pushed a major update to this PR that changes a lot.

As far as I understood the comments, the main problem was that I've moved code generation into the code writers. I now reverted this and also removed the HPyCCodeWriter. Code and its structure are now generated in the same places as before.

Additionally, there are two major additions compared to the previous changes:

  1. I'm now properly using HPyField to store objects in global variables.
  2. I migrated the majority of the module initialization code that sets up Cython's caching of builtins and other objects.

Also, where needed, I've changed the structure of the generated code such that it suits for both, C API and HPy. For example: For building tuples, in the C API you would just do:

PyObject *tuple = PyTuple_New(n);
PyTuple_SET_ITEM(tuple, 0, x);
PyTuple_SET_ITEM(tuple, 1, y);
...
/* use variable 'tuple' */

where in HPy you need to use a tuple builder:

HPyTupleBuilder builder = HPyTupleBuilder_New(ctx, n);
HPyTupleBuilder_Set(ctx, builder, 0, x);
HPyTupleBuilder_Set(ctx, builder, 1, y);
...
HPy tuple = HPyTupleBuilder_Build(ctx, builder);
/* use variable 'tuple' */

So, I've changed the structure in Cython such that it would work with the builder pattern but where the build step in C API is just a no-op.

In order to cover all possible use cases, I've introduced a Python class named Backend which is used to generate the API-specific code (very fine-grained). There are three API backends:

  1. CApiBackend: generate bare C API code (as close to the state before this PR as possible)
  2. HPyBackend: generate bare HPy API code
  3. CombinedBackend: uses meta-programming to generate code that suites for both, C API and HPy. One can decide for which API to compile just by defining (or not) macro "HPY". So, if the cythonized code will be included in an HPy extensions, this will automatically happen. This, for example, looks like that:
  __PYX_OBJECT some_function(__PYX_CONTEXT_DECL __PYX_OBJECT x) {
      int result;
      result = __PYX_LONG_AS_INT(__PYX_CONTEXT x);
  }

where __PYX_OBJECT is a macro that defines the Python object C type which is PyObject * for the C API and HPy for HPy. The CombinedBackend is probably the one that will in the end be the default but when we first merge changes, I would set the CApiBackend as default and this would generate code almost as before.

There are, of course, still some open points and tasks to be done before this can be merged. Here are some points I want to discuss:

  • How should the decision been made which backend to use? I suggest to introduce a compiler options like the language option.
  • Functions that return borrowed references and following increfs are in general a problem (in particular, function make_owned_reference doesn't work for HPy). Is it suitable to make all of them return a new reference?

fangerer avatar Feb 07 '22 10:02 fangerer

Functions that return borrowed references and following increfs are in general a problem (in particular, function make_owned_reference doesn't work for HPy).

Why not? Doesn't it just dup() a handle if necessary?

Is it suitable to make all of them return a new reference?

I'd like to avoid that if at all possible.

scoder avatar Feb 08 '22 00:02 scoder

in HPy you need to use a tuple builder

I wonder if we could just generate an inline helper function for each tuple length, in a new code writer section. There can't be that many different literal tuple or list sizes in a module, after all. The reference counting would still happen outside, but that function would then just steal a reference to each of its item arguments.

scoder avatar Feb 08 '22 00:02 scoder

Has there been more progress on this? Would love to see it! Great work!

HiperMaximus avatar Jul 21 '22 10:07 HiperMaximus

Hi @HiperMaximus , no, I'm sorry. I couldn't make progress on that since I was focusing on NumPy/HPy in the last months. @mattip tries to make some progress on this (see https://github.com/fangerer/cython/pull/1 ). We were also in the progress of figuring out how we can split this big change set into smaller pieces such that we can merge small chunks.

fangerer avatar Jul 25 '22 14:07 fangerer

Hi! I was wondering, is this feature still on the list and is it possible to see it in some way in the future?

igorcoding avatar Aug 12 '23 22:08 igorcoding

Hi @igorcoding! Yes, this is definitively something we (the HPy dev cores) want to do. This particular PR served as a proof-of-concept to explore ways how we can/should do it. Currently, I'm not personally working on this topic since I spend most of my HPy-time on HPy itself or on NumPy/HPy. However, a colleague started to work on it here.

fangerer avatar Aug 13 '23 17:08 fangerer

Hey @fangerer! This is awesome! Great to hear!

igorcoding avatar Aug 13 '23 17:08 igorcoding