[ESI] Add wrap function to Python API #893
Conversation
Just the ability to 'import' ESI into Python.
| #include "DialectModules.h" | ||
|
|
||
| #include "circt-c/Dialect/ESI.h" | ||
| #include "circt/Dialect/ESI/ESIDialect.h" |
There was a problem hiding this comment.
It seems to work based on the test case, but I think this pybind11 module is diverging from the notion of only layering on top of the CAPI. Specifically, the new resolvePortNames, and the existing buildESIWrapper functions are not part of the ESI CAPI.
I'm not sure if this is just lucky right now in terms of how the linking and RTTI and everything line up, or what. It might be worth adding a wrapper in the ESI CAPI layer to go through, instead of pointing to the dialect header/library directly.
There was a problem hiding this comment.
Yeah, I'm not familiar with the linking issues. I'm going through the C API for all the MLIR types, where there's a strong chance that linking issues could come up. The calls you're talking about are relatively simple C++ calls that are linked in the same way they would be in a C API. The C API function would actually be C++ code with C name mangling (and perhaps different linking mechanisms) so I'm not sure this is necessary.
@stellaraccident Can you comment on this? Will we run into linking issues by calling C++ code directly from a pybind11 wrapped function wherein I use the C API for Mlir* types, but don't go through a C API for simple C++ function calls? Near as I can tell, npcomp does that here (and presumably elsewhere).
There was a problem hiding this comment.
Since I think this is being done in npcomp, I don't think it's necessary to go through the C API. If we run into issues or Stella comments otherwise, we can change it.
| @@ -82,6 +82,72 @@ void circt::esi::findValidReadySignals( | |||
| } | |||
| } | |||
|
|
|||
| /// Given a list of logical port names, find the data/valid/ready port triples. | |||
| void circt::esi::resolvePortNames( | |||
There was a problem hiding this comment.
Seems like a lot of duplication here with findValidReadySignals. How do they differ? It seems like findValidReadySignals is using its heuristic to determine how data/valid/ready are named, whereas resolvePortNames lets the user specify the base name to look for?
To me it seems like resolvePortNames was a copied and tweaked, and they can be more unified. Could resolvePortNames call into findValidReadySignals? Is there a use-case (besides the tests) for findValidReadySignals if resolvePortNames exists as it does in this PR? At the least, it seems like a helper could be pulled out for "Find a port named like this", which they could both use, if it even makes sense to have the two separate APIs.
There was a problem hiding this comment.
Yeah, I was thinking about that while I was copying the code. My thought at the time is that the details are too different to make the additional complexity worth while. I've factored the common stuff out, and it's probably worth the additional complexity but not by much IMO.
As for the use case of findValidReadySignals, I think there is still one in wrapping handshake and other cases where you want to do the same thing on all of the LI ports (e.g. connect them all via cosim). In cases where you need to do different things, you kinda need to know the port names. There may be some exceptions based on the type.
There was a problem hiding this comment.
Thanks for doing the refactoring. It looks good to me.
|
@mikeurbach Thanks for the detailed review! |
This new Python API function takes a module and a list of ports then constructs and returns a ESI wrapper module. Sets up the infrastructure for the ESI Python API.
Thanks to @stellaraccident for all the help with the Python bindings!