• Remove suprious function

• Remove mlirModuleClone

• Address feedback

Test

Test

Fix comment

Slight tewak

Address feedback

Rebase

Missed one

Missed a few

Add Tests

Add context accessor on operation

Add test

It looks like there are three tests failing currently on master for me, but they also fail for the previous commit (8d73bee4edc2)
[build] Failed Tests (3):
[build] MLIR :: mlir-cpu-runner/async-group.mlir
[build] MLIR :: mlir-cpu-runner/async-value.mlir
[build] MLIR :: mlir-cpu-runner/async.mlir

It is great to see a unified approach to dialects, this will make further improvements much easier. Thanks, Stella!

Accidentally committed without "Differential" log: https://github.com/llvm/llvm-project/commit/5f65c4a8e6a9fbcbf45ff4cdf0b4815795dd4845

Whoops, I accidentally pushed without the "Differential" comment... anything I should do to clean this up?

Fix comments

LGTM. Thanks for doing this!

Only the helpers that are defined in the header are static inline. I think that the intent was to not exposing them in libMLIR.so and have them inlined in the client. I am not sure this is valid in C though: they has to be an external definition I believe and this would break if a client was taking the address.

The code in this revision should all be part of the TableGen generated code and not manually written.

That said, I think it would be nice to include a second dialect in the bindings as we work our way to a better system (and I need it for my test cases :-)

I very much agree @stellaraccident. I would even go one step further and say there should be an mlirGetDialectByNamespace(MlirStringRef) function to return this structure, which we can enable using static constructors (even though they are evil, this would be a very nice way to not have to create a CAPI for every dialect). For the record, in the Swift bindings I use "Dialect" to mean what you refer to as "Dialect Registration" and "Dialect Instance" to refer to the thing returned by getOrLoad. I also don't currently understand how Dialect Instances are useful (and why we can't just have "Dialect Registration" be the only thing (hence "Dialect")

I keep getting this error saying that the Debian build failed, though I don't see any errors:

Testing Time: 121.11s
  Unsupported      :   584
  Passed           : 40988
  Expectedly Failed:   174

Add some missing consts

Let's give Debian another chance...

Rebasing again

Rebasing to re-run CI

Rebasing to re-run CI

Try again after rebase

Fix diff

I'm guessing it is safe to ignore the unused function warnings? Also, It looks like the build failure was unrelated to the code change... is there any way to re-run a build?

Sorry I abandoned this because the situation I thought I was able to reproduce, stopped reproducing. I think the underlying issue was unrelated

The difference is that all the consumers of the C++ API are C++, so it is pretty straightforward to update when making breaking changes even if the surface is greater. Language bindings will live at least partially in their own ecosystem and requiring folks who work on the C API to understand each individual ecosystem may be too onerous, even if the surface area is smaller. To be clear, I’m just saying there is a tradeoff here, not advocating for us to decide on a particular point on that spectrum. I agree with Stella that we can wait until this becomes a bigger issue and then have a more formal discussion (hopefully with more data in the form of diverse language bindings).

Small process question: I authored the change to us MlirStringRef instead of const char *, what is the policy about breaking changes in the C API? I think it is likely unreasonable to expect any change in the C API to fix all in-tree language bindings, but since the Python bindings are in-tree, these types of changes can break them. I'm might be the only person working on non-Python bindings here, so I may be biased :)

Some more formatting fixes

Sorry for the spam, I am still learning my way around arc diff. This should be ready now though.

Use MlirStringRef throught the C API

Use MlirStringRef for all strings in the C API

Use MlirStringRef for all strings in the C API

Fix formatting

Use MlirStringRef throughout the API.

To be perfectly clear, I agree that we should use MlirStringRef throughout, but if we decide that we should do that across the API in one fell swoop (I’m happy to author that PR if folks are on board).

Removing spurious formatting changes

I have worked with many C APIs in a number of languages and the attitude of "just do the memory management bookkeeping in the host language" has created some very unfortunate complexity in the bindings and more than a few bugs. This is a very low-cost change for the C bindings, and doesn't add much complexity for language bindings that don't care about this functionality (they can just pass NULL to the extra arguments). I think this is also a win from a self-documentation standpoint: before, I would need to go and figure out what are the different ways diagnostic handlers can be unregistered to understand the semantics. I would also need to worry about concurrency, specifically whether or not my handler is guaranteed to not be called after detach returns. I could read the code to figure this out (it turns out, the answer is "no") but I think it is better to minimize this sort of spelunking and just say "we will call your handler and when we are done calling your handler we will call your callback".

Address feedback

If a host language is registering dialect handlers that may need to be cleaned up (for instance, via a reference count decrement), it theoretically _could_ keep a list of attached handlers and manually release them on detach or when the context is destroyed. deleteUserData is a simple way to eliminate this bit of bookkeeping and make "ownership" a bit more formal in this case, namely the context is responsible for telling you when it is done using the handler (and thus, the userData). This way, if we hypothetically add a setHandlers method which takes an array of handles and _replaces_ the current handler stack, then the host language wouldn't need to account for this, deleteUserData would just be called when the handler is no longer in use.

@mehdi_amini I added one more bit of functionality here, so you may want to take another look.

Added deleteUserData callback

Whoops, thank you for catching this... these must have disappeared when I was fixing MLIR_CAPI_EXPORTED-related merge conflicts