Introduce an initial version of C API for MLIR core IR components: Value, Type, Attribute, Operation, Region, Block, Location. These APIs allow for both inspection and creation of the IR in the generic form and intended for wrapping in high-level library- and language-specific constructs. At this point, there is no stability guarantee provided for the API.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Seems like a reasonable starting point to me. Details can be elaborated as we go.
mlir/include/mlir-c/IR.h | ||
---|---|---|
85 | Is the goal to have explicit bindings for the DiagnosticEngine and route errors through that? | |
99 | This is a lot of arguments and will be hard to version. I'm wondering whether there is any benefit to taking the complexity of having an explicit params struct with an initialize method? All of the caveats of having such a thing apply (needing to either heap alloc it or version it, etc). | |
mlir/lib/CAPI/test.c | ||
1 ↗ | (On Diff #276083) | Did you mean to put this in a test directory? |
mlir/include/mlir-c/IR.h | ||
---|---|---|
85 | I haven't thought about diagnostics, but it should be possible to provide something to set it up on the context. | |
99 | I don't expect this to change much at this point, this would mean deep changes to the IR structure (we list the entire OperationState here). I still think it makes sense to have a wrapper struct because I expect most of these operands to go unused. | |
mlir/lib/CAPI/test.c | ||
1 ↗ | (On Diff #276083) | I put in the wrong place on purpose so folks complain in the review and suggest a better location :) |
Skimming through it, looks great!
mlir/include/mlir-c/IR.h | ||
---|---|---|
22 | Typo representaiton | |
34 | Nice! | |
66 | What is the naming convention for all the types here? Is this in line with the LLVM bindings or something? | |
93 | In general we should take some care about documenting the error handling for each of these APIs | |
99 | Seems like binding the OperationState (but it'd require heap allocation though, but it could be made reusable with a clean() method) | |
mlir/lib/CAPI/IR.cpp | ||
158 ↗ | (On Diff #276421) | This is a bit of a big hammer, I'd leave this into a dedicated API. |
mlir/lib/CAPI/IR.cpp | ||
---|---|---|
158 ↗ | (On Diff #276421) | And now than that: it should also be in its own library. It is pretty likely that on smaller devices, distributors will replace whatever decision gets made here with a completely custom version that gives them exactly what they need. |
Updates Lgtm, modulo vote on unsigned
mlir/docs/CAPI.md | ||
---|---|---|
60 | I think you are missing a "default" word | |
93 | s/undex/index/ | |
mlir/include/mlir-c/IR.h | ||
140 | I'd need to look at compiler explorer to determine efficiency: I feel like this struct may be an exception that calls for an mlir OperationStateInit(MlirOperationState*) |
mlir/docs/CAPI.md | ||
---|---|---|
8 | nit: have a harder time | |
19 | nit: Is it useful to sort these alphabetically? | |
29 | nit: supposed -> intended? | |
44 | typo: takes inspects | |
96 | nit: should -> does? | |
mlir/include/mlir-c/IR.h | ||
72 | nit: Could we change Operations to something like Methods or API? The Operations part is confusing given the presence of an Operation class. | |
231 | typo: regions -> blocks? | |
mlir/lib/CAPI/IR.cpp | ||
43 ↗ | (On Diff #280173) | nit: static? |
50 ↗ | (On Diff #280173) | nit: You could also use llvm::None |
52 ↗ | (On Diff #280173) | nit: Add a message to these? |
72 ↗ | (On Diff #280173) | Can we align these comment blocks with the ones in the header? |
mlir/test/CAPI/ir.c | ||
160 | Extra semicolon here. |
mlir/include/mlir-c/IR.h | ||
---|---|---|
140 | It looks like the struct is constructed in the caller's stack even at -O0. |
This is looking good! Thanks for the work!
Also, I would like to use it for some of my coming work, so I am gently pinging the reviewers.
mlir/docs/CAPI.md | ||
---|---|---|
55 | This method doesn't seem to exist in this patch. Maybe change to another example? | |
mlir/lib/CAPI/IR.cpp | ||
129 ↗ | (On Diff #280366) | when is this allocation freed? |
This looks really fantastic Alex, you really nailed the details on this! The docs look great, the choice of intptr_t is great, the naming structure and consistency is A+. Thank you!
One question for future patches: how do you plan to handle layering with other libraries in MLIR? Should this be something like "CAPI/IR/..." to allow a "CAPI/ExecutionEngine" sort of API? We don't want the C API to depend (in a build system sense) on all the MLIR libraries that are eventually wrapped.
-Chris
I was thinking about having one file per "library", e.g. CAPI/IR.cpp, CAPI/ExecutionEngine.cpp, but you are right that library layering is not obvious in this case. We already have two separate libraries for IR and Registration, so it looks worth it to have lib/CAPI/IR/IR.cpp and lib/CAPI/Registration/Registration.cpp to better match the library structure.
Yeah makes sense. One file per library should work fairly well in practice I think, given how .a files work. The only downside of this that I see is that the build for unit tests and other things in-tree that depend on the C API will be bottlenecked on building everything - this could add to the critical path of the build.
nit: have a harder time