This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Initial version of C APIs
ClosedPublic

Authored by ftynse on Jul 7 2020, 8:00 AM.

Details

Summary
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.

Diff Detail

Event Timeline

ftynse created this revision.Jul 7 2020, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2020, 8:00 AM
stellaraccident accepted this revision.Jul 7 2020, 11:47 PM
stellaraccident added a subscriber: stellaraccident.

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?

This revision is now accepted and ready to land.Jul 7 2020, 11:47 PM
ftynse updated this revision to Diff 276421.Jul 8 2020, 7:10 AM

a slightly different implementation

ftynse marked 3 inline comments as done.Jul 8 2020, 7:23 AM
ftynse added inline comments.
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 :)

stellaraccident added inline comments.Jul 8 2020, 7:52 PM
mlir/include/mlir-c/IR.h
99

Sounds good. A struct on the stack would be good.

mlir/lib/CAPI/test.c
1 ↗(On Diff #276083)

Haha - well, having taken the bait, how about putting it in test/lib/CAPI?

Jing added a subscriber: Jing.Jul 8 2020, 8:49 PM

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.

stellaraccident added inline comments.Jul 9 2020, 7:32 AM
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.

ftynse updated this revision to Diff 278734.Jul 17 2020, 5:33 AM

Remove owning structs

ftynse updated this revision to Diff 280173.Jul 23 2020, 9:33 AM

Add docs, move tests.

ftynse marked 10 inline comments as done.Jul 23 2020, 9:35 AM
ftynse added inline comments.
mlir/include/mlir-c/IR.h
66

Defined the naming convention in the doc.

mlir/lib/CAPI/IR.cpp
158 ↗(On Diff #276421)

For now, I put it into different header+source, we will figure out the library separation when we get there.

ftynse retitled this revision from [mlir][WIP] C APIs to [mlir] Initial version of C APIs.
ftynse edited the summary of this revision. (Show Details)
stellaraccident accepted this revision.Jul 23 2020, 9:55 AM

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*)

rriddle added inline comments.Jul 23 2020, 10:44 AM
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.

ftynse updated this revision to Diff 280356.Jul 24 2020, 1:27 AM
ftynse marked 2 inline comments as done.

more cmake

ftynse updated this revision to Diff 280366.Jul 24 2020, 1:50 AM
ftynse marked 15 inline comments as done.

Review

ftynse marked an inline comment as done.Jul 24 2020, 2:36 AM
ftynse added inline comments.
mlir/include/mlir-c/IR.h
140

https://godbolt.org/z/cjafzc

It looks like the struct is constructed in the caller's stack even at -O0.

Jing added a comment.Jul 29 2020, 11:16 PM

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?

ftynse updated this revision to Diff 282187.Jul 31 2020, 4:15 AM

Use intptr_t

ftynse marked 2 inline comments as done.Jul 31 2020, 4:17 AM
ftynse added inline comments.
mlir/docs/CAPI.md
55

There are no methods that follow this rule in this patch... We still want to define the rule.

mlir/lib/CAPI/IR.cpp
129 ↗(On Diff #280366)

in mlirOperationCreate

stellaraccident accepted this revision.Aug 3 2020, 8:33 AM

Thank you. Updates Lgtm.

lattner accepted this revision.Aug 3 2020, 9:51 AM

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

ftynse marked 2 inline comments as done.Aug 3 2020, 10:00 AM

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.

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.

ftynse updated this revision to Diff 283206.Aug 5 2020, 5:38 AM

Move implementation files into separate directories to make the build cleaner

This revision was automatically updated to reflect the committed changes.