RFC: Adding a backend for HPy. by fangerer · Pull Request #4490 · cython/cython

fangerer · 2021-12-06T11:16:33Z

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).

…sign

fangerer · 2022-02-07T10:15:05Z

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:

I'm now properly using HPyField to store objects in global variables.
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:

CApiBackend: generate bare C API code (as close to the state before this PR as possible)
HPyBackend: generate bare HPy API code
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?

scoder · 2022-02-08T00:05:28Z

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 · 2022-02-08T00:12:56Z

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

Thanks again. This looks much, much better.

There is still a lot of code duplication in the utility functions, but I guess some of this can't be avoided without losing in readability. Still, my general feeling is that duplication is visibly more costly than macros.

Regarding the Backends, I'm sure there's still more that can be done by the C preprocessor, rather than cluttering Cython's code base with backend.do_this() calls. Why not have a __Pyx_CLEAR_GLOBAL() and a __Pyx_VISIT_GLOBAL()? C compiler macros are very local and cheap. They can even reduce the complexity if we give them good names. In contrast, new concepts in the compiler design are expensive.

I don't mind adding the HPy context argument macro to all calls. It really doesn't hurt. And a lot of it can still be hidden by appropriate C-API function macros. We have __Pyx_* variants for almost all of the C-API (that we use) by now. Why not just make it all of it? It may even help in supporting subinterpreters in CPython at some point. If you really feel like the context should be hidden, add a code.get_capi_call(function_name, args). Can't say if that makes it better.

Regarding the utility C code, I understand that you generally want to move the HPy case second, probably because you expect the non-HPy case to be more important and better known. However, I find negated conditions harder to read than positive conditions. It's clearer to write #if HPY followed by the HPy block, than #if ! HPY and then lots of other stuff followed by the HPy block. Maybe adding a /* HPy */ marker might also help already, can't say without seeing it. But I wouldn't mind putting the HPy case first if that helps readability.

scoder · 2022-02-07T21:59:25Z

+#if !CYTHON_COMPILING_IN_HPY
    if (likely(PyLong_Check(x))) {
+#else /* CYTHON_COMPILING_IN_HPY */
+    if (likely(HPy_TypeCheck($hpy_context_cname, x, $hpy_context_cname->h_LongType))) {
+#endif /* CYTHON_COMPILING_IN_HPY */


This looks like it could also be hidden by a macro, __Pyx_PyLong_Check(). The context is "just there", right? There is precedence for this in the exception handling code that passes the thread state instead of looking it up, when the CYTHON_FAST_THREAD_STATE feature flag is defined.

OTOH, you are using HPyLong_Check() elsewhere. Is this different?

scoder · 2022-02-07T22:08:14Z

 #if CYTHON_ASSUME_SAFE_MACROS
 #define __pyx_PyFloat_AsDouble(x) (PyFloat_CheckExact(x) ? PyFloat_AS_DOUBLE(x) : PyFloat_AsDouble(x))
-#else
+#else /* CYTHON_ASSUME_SAFE_MACROS */
 #define __pyx_PyFloat_AsDouble(x) PyFloat_AsDouble(x)
-#endif
+#endif /* CYTHON_ASSUME_SAFE_MACROS */


If you want to introduce these comments (which will get copied into the generated C files as it stands), then it would be good to at least repeat the actual condition. Here, you say #if FLAG and #else /* FLAG */, whereas in other places, you say #if ! FLAG and #else /* FLAG */. I think that undermines the intention of making it clearer which case we're looking at. I still have to look at the initial condition to see what the #else means.

I'm generally not a fan of these comments as they tend to suffer from bit rot quickly.
To me, they also make the #else and #endif more difficult to spot since they add a lot of verbosity to the line.

For the CYTHON_COMPILING_IN_HPY condition, maybe you can just add an /* HPY */ marker to the HPy case.

scoder · 2022-02-07T22:13:59Z

+class CombinedBackend(APIBackend):
+    name = "combined"
+    pyobject_ctype = "__PYX_OBJECT_CTYPE"
+    pyobject_ctype_base_part = "__PYX_OBJECT_CTYPE"


Seeing how ubiquitous this type is, let's make it less verbose and less screaming. Something like __Pyx_Object or __Pyx_PObject seems fine. It's usually clear from the context that it refers to a type.

scoder · 2022-02-07T22:21:28Z

 }
-#endif
+#else /* CYTHON_COMPILING_IN_HPY */
+static int __Pyx_InitString(HPyContext *ctx, __Pyx_StringTabEntry t, HPyField *str) {


Most of the differences can be handled with macros, and the rest are temporary differences that will go away as HPy matures. I'd rather not have three copies of (mostly) this function when two feel already too many.

scoder · 2022-02-07T22:24:07Z

-#if !CYTHON_COMPILING_IN_PYPY
+#if !CYTHON_COMPILING_IN_PYPY && !CYTHON_COMPILING_IN_HPY
 static {{c_ret_type}} __Pyx_PyInt_{{'' if ret_type.is_pyobject else 'Bool'}}{{op}}{{order}}(PyObject *op1, PyObject *op2, long intval, int inplace, int zerodivision_check); /*proto*/
-#else
+#elif CYTHON_COMPILING_IN_PYPY && !CYTHON_COMPILING_IN_HPY


If HPy is always different here, regardless of the runtime environment (PyPy/CPython), why not move it first? That avoids duplicating the condition.

scoder · 2022-02-08T00:17:46Z

        code.putln("#elif CYTHON_COMPILING_IN_LIMITED_API")
+
+        backend.put_hpy(code, '%s = _h2py(init_%s_impl(_HPyGetContext()));' % (
+            env.module_cname,
+            env.module_name))


In the limited API case?

That's, of course, not the primary condition.
The generated structure is:

#if CYTHON_PEP489_MULTI_PHASE_INIT // HPy does not yet support multi-phase init #else #if PY_MAJOR_VERSION < 3 // HPy does not support Python 2 #elif CYTHON_COMPILING_IN_LIMITED_API // this will always be true for HPy #else ...

So, it's currently the only valid case for HPy but I agree, it should be future-proof. I'll restructure that.

scoder · 2022-02-08T00:20:59Z

        code.globalstate.directives = old


+class CPPNode(Node):


CPP generally suggests C++, not C PreProcessor.

Right, I'll rename that but the node should eventually go away.

scoder · 2022-02-08T00:29:12Z

-        code.putln("PyObject* values[%d] = {%s};" % (
-            max_args, ','.join('0'*max_args)))
+        code.putln("%s values[%d] = {%s};" % (
+            backend.pyobject_ctype, max_args, ','.join(('__PYX_NULL', ) * max_args)))


Places like this should use the constant in Naming.py.

scoder · 2022-02-08T00:31:54Z

+        if def_node.is_cyfunction:
+            return Nodes.CPPNode(def_node.pos, body=assmt, cond="!CYTHON_COMPILING_IN_HPY")


Is this a temporary work-around or intended as a general exclusion?

That's a temporary workaround since this is not yet supported in HPy. In general, I need to think about __pyx_CyFunctionObject and how it plays together with HPy.
As far as I understand, __pyx_CyFunctionObject is an extension to CPython's PyCMethodObject. We cannot do exactly the same (i.e. embed the some struct) because HPy does, ofc, not expose the C-level struct of a method.
We can still define the ´__pyx_CyFunctionType(without embeddingPyCMethodObject) but everything that's declared in PyCMethodObject` would then be an attribute read or something.

scoder · 2022-02-08T00:33:10Z

    #  is_pyobject           boolean     Is a Python object type
+    #  is_hpy                boolean     Is an HPy handle type


Do we need this distinction? (handle vs. object)

No, that's a left-over. I just overlooked it.

HiperMaximus · 2022-07-21T10:20:47Z

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

fangerer · 2022-07-25T14:53:13Z

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 fangerer#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.

igorcoding · 2023-08-12T22:00:15Z

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

fangerer · 2023-08-13T17:12:54Z

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.

igorcoding · 2023-08-13T17:51:09Z

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

mattip and others added 30 commits July 11, 2021 22:15

DOC: add documentation for total_ordering

fbacec6

first cut at a hpy test

cdadb9a

make runtests.py import hpy and add the hpy include path

a28ffc0

implement module initialization

99ebea8

module initialization code generation and importing seems to work

1715e77

add hpy to requirements.txt

9bdea3b

Add class attr 'is_hpy' to Node

a8cb0d6

Generate HPy code for BinOpNode

b51c682

Fix: invalid C code for empty HPy defines

8c60d1a

Temporarily introduce HPyType

37147ea

Add HPy method flags

48f7e88

Add default name for HPy context argument

458d4bc

Introduce HPyCCodeWriter and move more code gen to it

8535a0c

Fix: HPy method flag keywords contains varargs

a32b08e

Use macro as init value for Python object type in scope

d1ed401

Add type c_hpy_ssize_t

4c04956

Add type c_hpy_ptr_type

46d5df0

Introduce mapping between C API and HPy types

a8c82b3

Generate correct signature for HPy keywords func

f1c8e80

Introduce code parts for HPy; protect them by ifdefs

7ca36c0

Generate func var decls via code writer

f1bc781

Move argument declaration generation to code writer

69a6219

Move error value definition to code writer

853bc29

Don't use module state for HPy

0bf6b3f

Use put_binary_call_with_error in BinOpNode

ad61800

Introduce sections (hpy_)string_obj_decls

49bbc62

Add HPy version of string table

4407a5b

Fix: forgot to pass ctx to HPy_Hash

22cc144

Fix: use HPy_IsNull

a85f516

Introduce section hpy_init_globals

2e3ddb9

fangerer added 15 commits February 1, 2022 16:22

Migrate function __Pyx_PyObject_IsTrue (and calls to it) to HPy

78f6150

Migrate snippet PyIntCompare to HPy

7840507

Add HPy helper function _HPy_IsSubclass

59f5bdd

Migrate snippet RaiseException to HPy

39a3e3d

Do not create CyFunction in HPy

ccce3ea

Don't use refnanny by default when loading/storingglobals

9a1fbe2

Fix: add missing opening brace

7e39ae1

Fix: used wrong function to generate methoddef entry

a41b16b

Use Py_XINCREF for possibly NULL fq_module_name

d84e49e

Add missing macro __Pyx_XDECREF_NO_REFNANNY

0c0dc7d

Always dup None ref in NoneNode

c80b5ae

Use put_newref in ReturnStatNode instead of make_owned_reference + as…

751253a

…sign

Share cleanup code in __Pyx_Raise

32348b0

Add reference to HPyErr_Format issue where appropriate

b1548a0

Remove invalid close

f69934f

fangerer force-pushed the hpy branch from ed82fd7 to f69934f Compare February 7, 2022 10:20

scoder reviewed Feb 8, 2022

View reviewed changes

scoder linked an issue Feb 8, 2022 that may be closed by this pull request

[ENH] Preliminary support for HPy #4197

Open

GalaxySnail mentioned this pull request Mar 20, 2022

Remove python2 compat codes, introduce dataclasses and type hints for bin/update-tables.py jquast/wcwidth#58

Closed

mattip mentioned this pull request May 2, 2022

C++ does not like the HPyTuple_Pack macro hpyproject/hpy#318

Closed

fangerer mentioned this pull request May 5, 2022

Improve the documentation for the 0.0.4 release hpyproject/hpy#313

Merged

antocuni mentioned this pull request Jul 22, 2022

HPy support psycopg/psycopg#154

Open

		if def_node.is_cyfunction:
		return Nodes.CPPNode(def_node.pos, body=assmt, cond="!CYTHON_COMPILING_IN_HPY")

		# is_pyobject boolean Is a Python object type
		# is_hpy boolean Is an HPy handle type

Uh oh!

Conversation

fangerer commented Dec 6, 2021

Uh oh!

fangerer commented Feb 7, 2022

Uh oh!

scoder commented Feb 8, 2022

Uh oh!

scoder commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scoder Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scoder Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HiperMaximus commented Jul 21, 2022

Uh oh!

fangerer commented Jul 25, 2022

Uh oh!

igorcoding commented Aug 12, 2023

Uh oh!

fangerer commented Aug 13, 2023

Uh oh!

igorcoding commented Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scoder commented Feb 8, 2022 •

edited

Loading

scoder Feb 7, 2022 •

edited

Loading

scoder Feb 8, 2022 •

edited

Loading

igorcoding commented Aug 13, 2023 •

edited

Loading