Adds the ability to create dynamic passes using the C-API. This allows passes
to be written in C or languages that use the C-bindings.
Details
- Reviewers
rriddle - Commits
- rG2387fadea3a8: [mlir][capi] Add external pass creation to MLIR C-API
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LG overall, I suggested another name for this inline.
mlir/include/mlir-c/Pass.h | ||
---|---|---|
123 | We probably should expand the doc here, this isn't a "core context" for which we can just point to the C++ doc. I would rather name this differently than "Dynamic Pass" because it's too close to the dynamic pipeline we have for passes. What about "External Passes" or "Pluggable Pass"? //===----------------------------------------------------------------------===// // External Pass API. // // This API allows to define passes outside of MLIR, not necessarily in // C++, and register them with the MLIR pass management // infrastructure. // //===----------------------------------------------------------------------===// | |
125 | ||
127 | I would even point to the C++ method also by adding something like "(see Pass::initialize())" |
mlir/include/mlir-c/Pass.h | ||
---|---|---|
123 | I like External Pass, it makes the use case pretty clear | |
127 | There is a 1-1 mapping except that initialize and deinitialize map to the C++ constructor and destructor. Maybe construct and destruct would be a more clear mapping? | |
134 | The result is used to signal the pass failure. I suppose I can add an argument to run that is a pointer to a bool or MlirLogicalResult that can be set to signal pass failure. Otherwise I can add callback argument to run that would directly call signalPassFailure, which would need to be called with a pointer to the C++ pass class that would also be passed to run. That approach seems a bit cumbersome but does model the C++ API more closely. Which approach would you prefer? |
mlir/include/mlir-c/Pass.h | ||
---|---|---|
127 | Ah right, that's confusing with the initialize() method that I believe exists on passes (should it be exposed here as well by the way?) |
mlir/include/mlir-c/Pass.h | ||
---|---|---|
127 | Oh, I missed that Pass::initialize() exists. Seems like that it is worth exposing as an optional callback. |
Renamed DynamicPass to ExternalPass.
Renamed initialize/deinitialize to construct/destruct.
Exposed Pass::initialize.
Addressed nits.
mlir/include/mlir-c/Pass.h | ||
---|---|---|
134 | I think that passing in the pointer of the MLIR pass itself would likely be better in the long run. This API will/may likely be expanded to expose access to things like analyses, which would also require access to the pass pointer itself (same with running nested pass managers, or various other types of Pass features). What do you think of that approach? |
Sorry for the late review, I just came back from OOO this week.
mlir/include/mlir-c/Pass.h | ||
---|---|---|
157 | ? | |
mlir/lib/CAPI/IR/Pass.cpp | ||
120 | nit: Drop the trivial braces, also can you spell out auto here? | |
mlir/test/CAPI/pass.c | ||
284 | Technically TypeIDs are supposed to be aligned by 8. I don't think anything would break right now, but it could. |
Added MlirExternalPass to external pass run callback, which can be used to
signal pass failure.
Added alignment check to mlirTypeIDCreate.
Addressed nits.
mlir/include/mlir-c/Pass.h | ||
---|---|---|
134 | That makes sense for future flexibility. I ended up exposing the pass in the callback as a separate MlirExternalPass type so it can be safely casted to the c++ ExternalPass type and call signalPassFailure (which was otherwise protected on Pass). I kept the return type of mlirCreateExternalPass as MlirPass since its only valid to directly use it as a generic pass but that might be kind of confusing. | |
mlir/test/CAPI/pass.c | ||
284 | I updated these to use aligned_alloc and added an assertion in mlirTypeIDCreate |
Add bindings for TypeIDAllocator, use in c api tests.
This works around Windows not having c type alignment apis available.
mlir/include/mlir-c/Pass.h | ||
---|---|---|
124 | ||
mlir/include/mlir/CAPI/IR.h | ||
31 ↗ | (On Diff #419814) | Should this rather be in Support.h? (Not sure what the layering here is supposed to be given that TypeID is also in this file) |
mlir/lib/CAPI/IR/IR.cpp | ||
795 | Will this still hold if some low bits are stolen? (Not sure if that would happen much/ever for this CAPI). | |
mlir/lib/CAPI/IR/Pass.cpp | ||
87 | Missing namespace comment here. | |
88 | Is there a downside to sinking this below the ExternalPass definition? | |
113 | Can you spell out auto here? | |
127 | Can we drop the llvm::? | |
137 | Can we spell out auto here? |
Address comments
mlir/lib/CAPI/IR/IR.cpp | ||
---|---|---|
795 | This api is for the initial creation of a type id from outside of MLIR, so no bits should have been stolen yet. My understanding is the bit-stealing could only happen to already created type ids. | |
mlir/lib/CAPI/IR/Pass.cpp | ||
87 | Weird how clang-format didn't pick up on that, added. | |
88 | I need it before ExternalPass is defined since ExternalPass::runOnOperation uses the wrap function generated by this macro. |
mlir/include/mlir/CAPI/IR.h | ||
---|---|---|
31 ↗ | (On Diff #419814) | We'd have to move both MlirTypeID and MlirTypeIDAllocator to Support.h since the allocate function returns an MlirTypeID as opposed to a pointer to one (so it couldn't be forward declared). I'm not sure if Support.h is a better place considering TypeID is a core MLIR data structure as opposed to auxiliary but I don't have a strong notion about the best place for it. |
mlir/include/mlir/CAPI/IR.h | ||
---|---|---|
31 ↗ | (On Diff #419814) | I mentioned it because TypeID is defined in the Support library(same as LogicalResult), not IR. |
mlir/include/mlir/CAPI/IR.h | ||
---|---|---|
31 ↗ | (On Diff #419814) | Oh, in that case moving it seems like the right way to go |
mlir/test/CAPI/pass.c | ||
---|---|---|
515 | This function is leaking memory, seems to be missing a mlirModuleDestroy here. |
mlir/test/CAPI/pass.c | ||
---|---|---|
515 |
mlir/test/CAPI/pass.c | ||
---|---|---|
515 | Thanks! |
We probably should expand the doc here, this isn't a "core context" for which we can just point to the C++ doc.
I would rather name this differently than "Dynamic Pass" because it's too close to the dynamic pipeline we have for passes. What about "External Passes" or "Pluggable Pass"?
Something like this: