I've got some pretty strong evidence that this is indeed deadlocking during verification processing, but I can't quite explain why.

In D104207#2822970, @lattner wrote:

This is just using llvm::parallelForEachN (not doing anything particularly fancy) so I can't imagine how it would be different than other similar things using it. It is possible this is exposing a lower level problem in LLVM threading.

In any case, let me know how I can help. I'd prefer not to revert this though, as it is a significant speedup.

Thanks. Rename lgtm

Thanks for cleaning this up. I'm marking approve because I could be convinced about the naming but don't love it how it is spelled in this patch. Open to other opinions.

Some nits and notes for the future. I like that this enables scalars and is much simpler than where the capture work was headed.

This is good - thanks. I'm glad we were able to back away from the more complicated capture semantics and just directly represent scalars.

Thanks - this looks right to me.

In D104009#2812422, @bondhugula wrote:

In D104009#2812418, @mehdi_amini wrote:

I would have wrote: ..., int numPaths, const MlirStringRef *libPaths).

This is the kind of API that I expect the user to map to an ArrayRef on the other side: the C API is unsafe (by nature somehow...) but the chance for error should be almost none because this is only a "binding" API and safe constructs can be used to wrap around this.

Makes sense to me - this makes it cleaner at the call site both for C and Python. It's just a downside that you'd have a bad crash if there is a consistency b/w the count and the number of elements in the ref. The comma separated string avoids that by design and you never get an opaque crash in the C API. Note that the C API user won't typically have an ArrayRef or a higher level structure to map that to the C API. They are using C right? :-)

All else being equal, it is better to not be in the business of textually manipulating file paths with ad hoc separators. While a bit more typing, would you be open to changing the c API to take a size and pointer to an array of string views? Then on the python side, accept a list of strings.

Can we please have an explanation/rationale for reverts? Given the short time window, I'm left to assume something was done in error.

Thank you for the fix!

Thanks for updating the patch. I re-reviewed this and cross referenced with the spec, and lgtm. Left one comment for the future. Landing as-is.

Ah, you were using the web interface. The docs have extra flags that must be included for full context: https://llvm.org/docs/Phabricator.html#id5

Marking as accepted for when the running is right to land.

In D102958#2776297, @mehdi_amini wrote:

Can you upload patches with full context please? Right now I can't see which ops are modified here.

Nice improvement - thanks. Do you need me to land this for you?

Thanks!

Thank you - not having this was unintentional.

Thanks!

Thanks for the patch! I'm accepting this to help with timezones but would like my comments addressed/further discussed before landing.

This broke the windows build bot, and I missed it. Rolling forward fix in: https://reviews.llvm.org/D102189

PTAL

Comments

Rebase

Comments.

Addressed comments. Thanks.

Rebase and comments.

This has come along nicely since we looked at it together - thank you for spending the time on adding an additional layer of testing. Code lgtm - I'll leave to Nicolas wrt details of Linalg parse/print/etc.

Comments.

Fix issue where 'get' factory method was returning the base class.

Also adding Alex for a look, since I think this is the first such case (dialect attribute) we have in tree.

I found this change because one of our bots picked up the state prior and was failing, and upon seeing it, I had the same reaction as Mehdi. Thanks for the build-related fix, but it would also be good to look a bit more at this.

Alex: Made a change that might appease your machine. Let me know.

In D101493#2728298, @ftynse wrote:

This particular problem seems to come from the name clash between the types.py test and the types.py distributed with Python. It's strange that you don't hit the problem.

In D101493#2731131, @bondhugula wrote:

I think you are missing an update to docs/Bindings/Python.md? (The paths there have to change.)

Address comments and rebase.

Rebase

In D101493#2727037, @ftynse wrote:

Tests seem to fail for me locally

: 'RUN: at line 1';   /usr/bin/python3.9 /home/zinenko/llvm-project/mlir/test/python/ir/value.py | /home/zinenko/llvm-project/build2/bin/FileCheck /home/zinenko/llvm-project/mlir/test/python/ir/value.py
--
Exit Code: 2

Command Output (stderr):
--
Traceback (most recent call last):
  File "/home/zinenko/llvm-project/build2/python/mlir/__init__.py", line 23, in <module>
    import _mlir_libs
ModuleNotFoundError: No module named '_mlir_libs'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/zinenko/llvm-project/build2/python/mlir/_cext_loader.py", line 12, in <module>
    import _mlir_libs
ModuleNotFoundError: No module named '_mlir_libs'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/zinenko/llvm-project/mlir/test/python/ir/value.py", line 4, in <module>
    from mlir.ir import *
  File "/home/zinenko/llvm-project/build2/python/mlir/__init__.py", line 28, in <module>
    from ._dlloader import preload_dependency as _preload_dependency
  File "/home/zinenko/llvm-project/build2/python/mlir/_dlloader.py", line 6, in <module>
    import platform
  File "/usr/lib/python3.9/platform.py", line 117, in <module>
    import re
  File "/usr/lib/python3.9/re.py", line 124, in <module>
    import enum
  File "/usr/lib/python3.9/enum.py", line 2, in <module>
    from types import MappingProxyType, DynamicClassAttribute
  File "/home/zinenko/llvm-project/mlir/test/python/ir/types.py", line 4, in <module>
    from mlir.ir import *
  File "/home/zinenko/llvm-project/build2/python/mlir/ir.py", line 6, in <module>
    from ._cext_loader import _reexport_cext
  File "/home/zinenko/llvm-project/build2/python/mlir/_cext_loader.py", line 17, in <module>
    from ._dlloader import preload_dependency as _preload_dependency
ImportError: cannot import name 'preload_dependency' from partially initialized module 'mlir._dlloader' (most likely due to a circular import) (/home/zinenko/llvm-project/build2/python/mlir/_dlloader.py)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/zinenko/llvm-project/build2/bin/FileCheck /home/zinenko/llvm-project/mlir/test/python/ir/value.py

--

********************
********************
Failed Tests (15):
  MLIR :: python/ir/affine_expr.py
  MLIR :: python/ir/affine_map.py
  MLIR :: python/ir/array_attributes.py
  MLIR :: python/ir/attributes.py
  MLIR :: python/ir/context_lifecycle.py
  MLIR :: python/ir/context_managers.py
  MLIR :: python/ir/debug.py
  MLIR :: python/ir/dialects.py
  MLIR :: python/ir/insertion_point.py
  MLIR :: python/ir/integer_set.py
  MLIR :: python/ir/location.py
  MLIR :: python/ir/module.py
  MLIR :: python/ir/operation.py
  MLIR :: python/ir/types.py
  MLIR :: python/ir/value.py

In D101493#2727002, @mehdi_amini wrote:

Seems mechanical, LGTM on the principle.

Rebase.

Fix install.

In D101422#2724396, @stellaraccident wrote:

In D101422#2724350, @mikeurbach wrote:

I've updated the test case to show what I mean. The bottom line is I need to make sure the Python reference is dropped and liveOperations is cleaned out before I can create any more operations. In the test, the addition of Operation.create("custom.op2") would hit the previously linked assertion, unless I added the del op.

This is somewhat non-intuitive to me, and when I first hit this, I was hoping to just have erase take care of removing from from liveOperations. But I wonder if that's not the right approach. The assertion says "cannot create detached operation that already exists", but I am quite sure this is a new operation. I did some debugging, and indeed, when Operation.create("custom.op2") is called, the operation.ptr is the same one that was used when Operation.create("custom.op1") was created. I think this hasn't really come up because we have only been able to create new operations thus far. Is there something fundamental to change here, or is this just how creating operations in MLIR works?

If this is just how creating operations works, I'm back to my original idea of updating the assertion in the destructor. What if it asserted that there was 0 or 1 operation in the liveOperations map? If something, like erase, already did the work of removing it, shouldn't that be ok? This is an operation that is about to no longer exist, and the map will reflect that.

Ah, there are sharp edges there indeed. The ownership model for operations was set up so that we could have "invalid" PyOperation instances (i.e. their backing operation was either destroyed or otherwise of an unknown state) -- however, as with all things that are being exercised for the first time, I wouldn't be surprised at all to find that the aspiration was not completely implemented/bug free. I believe the right thing on erase is to:

• Remove the PyOperation from the live set.
• Set the PyOperation::valid bit to false.
• Have a quick look over PyOperation methods and make sure things are guarded correctly with checkValid(). I wouldn't be surprised if some are missing.
• Condition everything in PyOperation::~PyOperation() on if (valid) { ... }. This should have been done from the get-go and is a bug.

Once a backing MLIR Operation is deleted, by whatever means, it is not legal to keep a dangling pointer to it: just like anything in C++, this pointer can/will likely be handed out again by malloc and friends.

I can look more deeply at this if the above doesn't work/you need help (it will just be a context switch as there is subtlety here that I need to remember).

In D101422#2724350, @mikeurbach wrote:

I've updated the test case to show what I mean. The bottom line is I need to make sure the Python reference is dropped and liveOperations is cleaned out before I can create any more operations. In the test, the addition of Operation.create("custom.op2") would hit the previously linked assertion, unless I added the del op.

This is somewhat non-intuitive to me, and when I first hit this, I was hoping to just have erase take care of removing from from liveOperations. But I wonder if that's not the right approach. The assertion says "cannot create detached operation that already exists", but I am quite sure this is a new operation. I did some debugging, and indeed, when Operation.create("custom.op2") is called, the operation.ptr is the same one that was used when Operation.create("custom.op1") was created. I think this hasn't really come up because we have only been able to create new operations thus far. Is there something fundamental to change here, or is this just how creating operations in MLIR works?

If this is just how creating operations works, I'm back to my original idea of updating the assertion in the destructor. What if it asserted that there was 0 or 1 operation in the liveOperations map? If something, like erase, already did the work of removing it, shouldn't that be ok? This is an operation that is about to no longer exist, and the map will reflect that.

In D101496#2724341, @jdd wrote:

Thanks @stellaraccident. So I assume you'd be opposed to making this a real public API by moving some of the python headers into 'mlir/include'? I was going to do that next since as of now, it's quite awkward to #include in out-of-tree code.

In D101496#2724291, @mikeurbach wrote:

PSA noted. When I mentioned this on discourse, it seemed like this and the previous change for attributes were expansions we could get some mileage out of without totally going off the deep end. Are there immediate linkage concerns with deriving PyConcreteType out of tree, or just the general concerns we have been discussing?

Works for me, but I will make the PSA that by expanding the public API in this way, we are heading in to really deep waters when it comes to linkage issues and bugs. We can run this forward a bit and see, but know that actually shipping a shared mlir python package in combination with other projects has a lot of sharp edges that will need resolution at some point.

This looks good to me - will leave the rest of the review to Nicolas. We do have docs in the docs/ tree for this language, and it would be good to update them.

I need to study this a bit: there is some subtlety to the ownership model. In the mean time, I'm trying to imagine the use case for needing this and failing. Can you share more of the motivation?