backend: (llvm) add ctypes conversion registry [STACKED PR]#6183
backend: (llvm) add ctypes conversion registry [STACKED PR]#6183sueszli wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6183 +/- ##
=======================================
Coverage 86.85% 86.86%
=======================================
Files 428 429 +1
Lines 64253 64294 +41
Branches 7369 7373 +4
=======================================
+ Hits 55808 55849 +41
+ Misses 6880 6879 -1
- Partials 1565 1566 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
945c295 to
6d33211
Compare
superlopuh
left a comment
There was a problem hiding this comment.
This is great, but I'm not sure that this is the optimal design for the mechanism. As it stands, the system is closed, and only the types added in the function can be mapped. I think two other options would be better:
A) have an abstract CTypeConvertible abstract superclass, that all the relevant attributes subclass,
B) have some kind of mapping ctx thing that lets users register conversion functions like we do for the frontend.
My gut feeling says to go for B, even if it's more infrastructure, as then the builtin dialect doesn't have to know about ctypes conversion, and the ctypes conversion doesn't need to know about the builtin dialect, only a third file that can bridge the two.
What do you think?
Also, it feels like we should have a bidirectional conversion, not sure whether this is a conversation for now or for later.
BTW This is the sort of thing where it would first be useful to have a bigger PR that would have an example of the final use-case, to be able to have informed API design discussions based on usage. In this case, we have the JIT prototype in the filecheck folder, maybe that could be updated in this PR also?
|
Sasha Feedback:
|
|
Thank you for reviewing and providing clear feedback. Really appreciate it.
Is it OK for me to turn this into an initial draft / stacked PR that we then later break down into smaller PRs?
So something similar to PyASTContext? xdsl/xdsl/frontend/pyast/context.py Lines 49 to 100 in a44f0a7
Sure, we could come back to this at a later point. Thanks |
|
I vibed this first draft (and rigerously reviewed). Does this kind of align with what you had in mind? Thanks |
| value = self.registry[type(type_attr)] | ||
| except KeyError: | ||
| raise LLVMTranslationException(f"No ctypes mapping for type: {type_attr}") | ||
| is_converter = callable(value) and not isinstance(value, type) |
There was a problem hiding this comment.
Do you prefer comments or named conditions?
There was a problem hiding this comment.
a) having the var is_converter = callable(value) and not isinstance(value, type)
b) inlining callable(value) and not isinstance(value, type) with a comment that explains what it is
|
|
||
|
|
||
| @ctx.jit | ||
| def plus(a: float, b: float) -> float: |
There was a problem hiding this comment.
note that these are not ctypes.c_double, so it's not just translation to/from ctypes that we need here. Although it seems like there's some magic for floats specifically.
There was a problem hiding this comment.
Ok will keep in mind when we work on non-scalar types. For now, should I add a comment?
There was a problem hiding this comment.
What I mean is that for this example technically it's a regression because previously the jit function had float in the type, which is handled. Now that it's P, R, we've lost any kind of support to detect what's handled and what isn't. Ideally we'd not have this kind of regression, and have a context that would map python objects to ctypes and back to make sure that the thing will either be ill-typed or raise an error if it's used for things that aren't handled.
There was a problem hiding this comment.
Oh, this flew right past me. I added a comment.
that would map python objects to ctypes and back
Now it makes sense. And it has to be both ways because we also then marshall the result back (unpack the result from its ctypes form back into a Python value), right?
There was a problem hiding this comment.
Sorry, I think I wasn't clear before. This is exactly what I meant. In a way, the ctypes mapping of types is still crucial, but it would help to also complement it with a typing of python values to ctypes values to complete this translation.
There was a problem hiding this comment.
Where would the python_type_context be placed (xdsl.jit.? jit.llvm.?)
4d6d3e8 to
fac45b9
Compare
Add CTypeContext, a registry mapping xDSL attribute classes to either ctypes types or per-instance converter callables. register_builtin_ctypes installs the standard scalar mappings for the builtin and llvm dialects. Wire the registry into the pyjit prototype so the CFUNCTYPE signature is derived from llvm.func types instead of hardcoded.
Sasha's suggestion: hang JIT infrastructure off a top-level xdsl.jit package so future targets (WASM, in-xDSL JIT) don't have to live under xdsl.backend. The LLVM-specific layer becomes xdsl.jit.llvm.
| self.ctype_ctx = CTypeContext() | ||
| register_builtin_ctypes(self.ctype_ctx) | ||
|
|
||
| # TODO: pair with a Python-type registry so unsupported P/R fail here, not deep in MCJIT. |
There was a problem hiding this comment.
no, let's do this in this PR, but keep it minimal
Essentially just follows the PyASTContext pattern but with CTYPEs.