This is an archive of the discontinued LLVM Phabricator instance.

[mlir][capi] Add dynamic pass creation to MLIR C-API
ClosedPublic

Authored by trilorez on Mar 16 2022, 3:47 PM.

Details

Summary

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.

Diff Detail

Event Timeline

trilorez created this revision.Mar 16 2022, 3:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 3:47 PM
trilorez requested review of this revision.Mar 16 2022, 3:47 PM
trilorez updated this revision to Diff 416212.Mar 17 2022, 9:32 AM

Rebase main, rename builtin.func to func.func

rriddle added inline comments.Mar 17 2022, 9:46 AM
mlir/include/mlir-c/Pass.h
127

Please use complete sentences for comments.

134

I would try to model more closely to the C++ API, which doesn't return a result.

mlir/lib/CAPI/IR/Pass.cpp
89

nit: Drop the newline here.

109

nit: Drop trivial braces here and below

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"?
Something like this:

//===----------------------------------------------------------------------===//
// 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())"
We have a 1-1 mapping here right?

trilorez added inline comments.Mar 17 2022, 10:47 AM
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?

mehdi_amini added inline comments.Mar 17 2022, 11:22 AM
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?)

trilorez added inline comments.Mar 17 2022, 11:49 AM
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.

trilorez updated this revision to Diff 416277.Mar 17 2022, 12:08 PM

Renamed DynamicPass to ExternalPass.
Renamed initialize/deinitialize to construct/destruct.
Exposed Pass::initialize.
Addressed nits.

trilorez updated this revision to Diff 416280.Mar 17 2022, 12:22 PM

Rebase main

trilorez added inline comments.Mar 29 2022, 10:26 AM
mlir/include/mlir-c/Pass.h
134

@rriddle Any thoughts on these approaches?

rriddle added inline comments.Mar 30 2022, 10:22 PM
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.

trilorez updated this revision to Diff 419524.Mar 31 2022, 11:32 AM

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

trilorez updated this revision to Diff 419781.Apr 1 2022, 9:13 AM

Fix formatting

trilorez updated this revision to Diff 419799.Apr 1 2022, 10:31 AM

Use _Alignas instead of aligned_alloc

trilorez updated this revision to Diff 419814.Apr 1 2022, 11:17 AM

Add bindings for TypeIDAllocator, use in c api tests.
This works around Windows not having c type alignment apis available.

rriddle added inline comments.Apr 1 2022, 1:38 PM
mlir/include/mlir-c/Pass.h
124
mlir/include/mlir/CAPI/IR.h
31

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
791

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?

trilorez updated this revision to Diff 419862.Apr 1 2022, 2:19 PM

Address comments

mlir/lib/CAPI/IR/IR.cpp
791

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.

trilorez added inline comments.Apr 1 2022, 2:40 PM
mlir/include/mlir/CAPI/IR.h
31

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.

rriddle added inline comments.Apr 1 2022, 2:42 PM
mlir/include/mlir/CAPI/IR.h
31

I mentioned it because TypeID is defined in the Support library(same as LogicalResult), not IR.

trilorez added inline comments.Apr 1 2022, 2:48 PM
mlir/include/mlir/CAPI/IR.h
31

Oh, in that case moving it seems like the right way to go

trilorez updated this revision to Diff 419873.Apr 1 2022, 3:25 PM

Move MlirTypeID and MlirTypeIDAllocator to Support.h

rriddle accepted this revision.Apr 4 2022, 12:56 AM
This revision is now accepted and ready to land.Apr 4 2022, 12:56 AM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.Apr 4 2022, 11:17 AM
mlir/test/CAPI/pass.c
515

This function is leaking memory, seems to be missing a mlirModuleDestroy here.

rriddle added inline comments.Apr 4 2022, 11:28 AM
mlir/test/CAPI/pass.c
515

Thanks!