User Details
- User Since
- Oct 20 2020, 10:14 AM (87 w, 5 d)
Jan 31 2022
May 24 2021
- Remove suprious function
- Remove mlirModuleClone
May 23 2021
- Address feedback
Test
Test
Fix comment
Feb 9 2021
Feb 8 2021
Slight tewak
Address feedback
Rebase
Missed one
Missed a few
Add Tests
Add context accessor on operation
Feb 7 2021
Feb 3 2021
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
Jan 9 2021
It is great to see a unified approach to dialects, this will make further improvements much easier. Thanks, Stella!
Dec 15 2020
Dec 3 2020
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
Nov 29 2020
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
Nov 28 2020
Rebasing to re-run CI
Rebasing to re-run CI
Nov 27 2020
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?
Nov 26 2020
Sorry I abandoned this because the situation I thought I was able to reproduce, stopped reproducing. I think the underlying issue was unrelated
Nov 24 2020
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 :)
Nov 23 2020
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.
Nov 21 2020
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).
Nov 20 2020
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
Nov 19 2020
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