Page MenuHomePhabricator

stellaraccident (Stella Laurenzo)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 20 2019, 9:16 PM (78 w, 1 h)

Recent Activity

Today

stellaraccident committed rG4b9d28bd530f: Partial rollback: Disable MLIR verifier parallelism. (authored by stellaraccident).
Partial rollback: Disable MLIR verifier parallelism.
Fri, Jun 18, 2:58 PM
stellaraccident closed D104570: Partial rollback: Disable MLIR verifier parallelism..
Fri, Jun 18, 2:58 PM · Restricted Project
stellaraccident added a reviewer for D104570: Partial rollback: Disable MLIR verifier parallelism.: lattner.
Fri, Jun 18, 2:44 PM · Restricted Project
stellaraccident requested review of D104570: Partial rollback: Disable MLIR verifier parallelism..
Fri, Jun 18, 2:44 PM · Restricted Project

Yesterday

stellaraccident added a comment to D104207: [Verifier] Parallelize verification and dom checking. NFC..

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

Thu, Jun 17, 11:51 PM · Restricted Project
stellaraccident added a comment to D104207: [Verifier] Parallelize verification and dom checking. NFC..

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.

Thu, Jun 17, 7:32 PM · Restricted Project

Wed, Jun 16

stellaraccident accepted D104321: [mlir] Move `memref.dim` canonicalization using `InferShapedTypeOpInterface` to a separate pass..

Thanks. Rename lgtm

Wed, Jun 16, 11:53 AM · Restricted Project

Tue, Jun 15

stellaraccident accepted D104321: [mlir] Move `memref.dim` canonicalization using `InferShapedTypeOpInterface` to a separate pass..

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.

Tue, Jun 15, 6:30 PM · Restricted Project
stellaraccident added inline comments to D104224: [mlir][linalg] Adapt yaml codegen to support scalar parameters..
Tue, Jun 15, 5:20 AM · Restricted Project

Mon, Jun 14

stellaraccident accepted D104224: [mlir][linalg] Adapt yaml codegen to support scalar parameters..

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

Mon, Jun 14, 9:53 PM · Restricted Project
stellaraccident accepted D104220: [mlir][linalg][python] Adapt the OpDSL to use scalars..

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

Mon, Jun 14, 9:44 PM · Restricted Project
stellaraccident accepted D103853: [MLIR] [Python] Add `owner` to PyValue and fix its parent reference.

Thanks - this looks right to me.

Mon, Jun 14, 9:37 PM · Restricted Project

Fri, Jun 11

stellaraccident added inline comments to D104009: [MLIR] Execution engine python binding support for shared libraries.
Fri, Jun 11, 12:36 AM · Restricted Project
stellaraccident added a comment to D104009: [MLIR] Execution engine python binding support for shared libraries.

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? :-)

Fri, Jun 11, 12:33 AM · Restricted Project

Thu, Jun 10

stellaraccident added a comment to D104009: [MLIR] Execution engine python binding support for shared libraries.

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.

Thu, Jun 10, 12:12 AM · Restricted Project

Fri, Jun 4

stellaraccident added inline comments to D103669: Avoid assertion failure on printing scf.for in debug dump..
Fri, Jun 4, 7:08 PM · Restricted Project
stellaraccident added a comment to D103724: Revert "Add memref.dim canonicalization patterns to TilingCanonicalizationPatterns".

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

Fri, Jun 4, 4:31 PM · Restricted Project
stellaraccident added inline comments to D103669: Avoid assertion failure on printing scf.for in debug dump..
Fri, Jun 4, 9:03 AM · Restricted Project

Thu, Jun 3

stellaraccident added a reviewer for D103669: Avoid assertion failure on printing scf.for in debug dump.: rriddle.
Thu, Jun 3, 10:35 PM · Restricted Project
stellaraccident requested review of D103669: Avoid assertion failure on printing scf.for in debug dump..
Thu, Jun 3, 10:35 PM · Restricted Project
stellaraccident added a reviewer for D103657: [NFC] Add ArrayRef includes to two files.: jpienaar.
Thu, Jun 3, 4:22 PM · Restricted Project
stellaraccident requested review of D103657: [NFC] Add ArrayRef includes to two files..
Thu, Jun 3, 4:20 PM · Restricted Project

Tue, May 25

stellaraccident accepted D103085: [mlir][CAPI][test] Change casts and fprintf format strings from long to intptr_t.

Thank you for the fix!

Tue, May 25, 8:39 AM · Restricted Project

Mon, May 24

stellaraccident committed rG96aa0a4115bc: Enable MLIR Python bindings for TOSA. (authored by stellaraccident).
Enable MLIR Python bindings for TOSA.
Mon, May 24, 11:07 AM
stellaraccident closed D103035: Enable MLIR Python bindings for TOSA..
Mon, May 24, 11:07 AM · Restricted Project
stellaraccident requested review of D103035: Enable MLIR Python bindings for TOSA..
Mon, May 24, 9:41 AM · Restricted Project
stellaraccident committed rG1ceff40df0a4: [mlir][tosa] Align tensor rank specifications with current spec (authored by sjarus).
[mlir][tosa] Align tensor rank specifications with current spec
Mon, May 24, 9:04 AM
stellaraccident closed D102958: [mlir][tosa] Align tensor rank specifications with current spec.
Mon, May 24, 9:04 AM · Restricted Project
stellaraccident added a comment to D102958: [mlir][tosa] Align tensor rank specifications with current spec.

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.

Mon, May 24, 9:02 AM · Restricted Project

Sun, May 23

stellaraccident added a comment to D102958: [mlir][tosa] Align tensor rank specifications with current spec.

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

Sun, May 23, 9:29 PM · Restricted Project
stellaraccident accepted D102997: [MLIR] Drop old cmake var names.

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

Sun, May 23, 8:39 PM · Restricted Project
stellaraccident added a comment to D102958: [mlir][tosa] Align tensor rank specifications with current spec.

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

Sun, May 23, 7:23 PM · Restricted Project
stellaraccident accepted D102958: [mlir][tosa] Align tensor rank specifications with current spec.

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

Sun, May 23, 6:48 PM · Restricted Project
stellaraccident accepted D102981: [MLIR] [Python] Add Operation.parent.

Thanks!

Sun, May 23, 6:46 PM · Restricted Project
stellaraccident accepted D102977: [MLIR] Update MLIR build config to reflect cmake variable renames.
Sun, May 23, 6:46 PM
stellaraccident accepted D102976: [MLIR] Make MLIR cmake variable names consistent.
Sun, May 23, 6:44 PM · Restricted Project

Sat, May 22

stellaraccident added inline comments to D102976: [MLIR] Make MLIR cmake variable names consistent.
Sat, May 22, 11:27 PM · Restricted Project

May 19 2021

stellaraccident accepted D102797: [mlir][CAPI] Expose [u]int8 DenseElementsAttr..
May 19 2021, 12:50 PM · Restricted Project
stellaraccident accepted D102800: [mlir] Add include guard to TOSA tblgen passes.

Thank you - not having this was unintentional.

May 19 2021, 12:49 PM · Restricted Project

May 16 2021

stellaraccident accepted D102551: [MLIR][PYTHON] Provide opt level for ExecutionEngine Python binding.

Thanks!

May 16 2021, 12:54 AM · Restricted Project

May 15 2021

stellaraccident accepted D102551: [MLIR][PYTHON] Provide opt level for ExecutionEngine Python binding.

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

May 15 2021, 10:44 AM · Restricted Project

May 12 2021

stellaraccident accepted D102362: [mlir][sparse][capi][python] add sparse tensor passes.
May 12 2021, 2:37 PM · Restricted Project

May 10 2021

stellaraccident committed rG295087644a46: [mlir] Fix windows build bot break due to use of `alloca` in a test. (authored by stellaraccident).
[mlir] Fix windows build bot break due to use of `alloca` in a test.
May 10 2021, 1:43 PM
stellaraccident committed rGa2c8aebd8f8f: [mlir][Python] Finish adding RankedTensorType support for encoding. (authored by stellaraccident).
[mlir][Python] Finish adding RankedTensorType support for encoding.
May 10 2021, 1:43 PM
stellaraccident closed D102189: [mlir] Fix windows build bot break due to use of `alloca` in a test..
May 10 2021, 1:43 PM · Restricted Project
stellaraccident closed D102184: [mlir][Python] Finish adding RankedTensorType support for encoding..
May 10 2021, 1:43 PM · Restricted Project
stellaraccident added a comment to D102141: [mlir][CAPI] Add CAPI bindings for the sparse_tensor dialect..

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

May 10 2021, 1:05 PM · Restricted Project
stellaraccident edited reviewers for D102189: [mlir] Fix windows build bot break due to use of `alloca` in a test., added: stella.stamenova; removed: aartbik.
May 10 2021, 1:04 PM · Restricted Project
stellaraccident requested review of D102189: [mlir] Fix windows build bot break due to use of `alloca` in a test..
May 10 2021, 1:03 PM · Restricted Project
stellaraccident added a comment to D102184: [mlir][Python] Finish adding RankedTensorType support for encoding..

PTAL

May 10 2021, 11:30 AM · Restricted Project
stellaraccident updated the diff for D102184: [mlir][Python] Finish adding RankedTensorType support for encoding..

Comments

May 10 2021, 11:30 AM · Restricted Project
stellaraccident updated the diff for D102184: [mlir][Python] Finish adding RankedTensorType support for encoding..

Rebase

May 10 2021, 11:09 AM · Restricted Project
stellaraccident committed rGf38633d1bbf5: [mlir][Python] Re-export cext sparse_tensor module to the public namespace. (authored by stellaraccident).
[mlir][Python] Re-export cext sparse_tensor module to the public namespace.
May 10 2021, 11:09 AM
stellaraccident closed D102183: [mlir][Python] Re-export cext sparse_tensor module to the public namespace..
May 10 2021, 11:09 AM · Restricted Project
stellaraccident updated the diff for D102183: [mlir][Python] Re-export cext sparse_tensor module to the public namespace..

Comments.

May 10 2021, 11:08 AM · Restricted Project
stellaraccident requested review of D102184: [mlir][Python] Finish adding RankedTensorType support for encoding..
May 10 2021, 11:06 AM · Restricted Project
stellaraccident requested review of D102183: [mlir][Python] Re-export cext sparse_tensor module to the public namespace..
May 10 2021, 10:43 AM · Restricted Project
stellaraccident committed rGf13893f66a22: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement… (authored by stellaraccident).
[mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement…
May 10 2021, 10:16 AM
stellaraccident closed D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding..
May 10 2021, 10:16 AM · Restricted Project
stellaraccident added a comment to D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding..

Addressed comments. Thanks.

May 10 2021, 10:00 AM · Restricted Project
stellaraccident updated the diff for D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding..

Rebase and comments.

May 10 2021, 10:00 AM · Restricted Project
stellaraccident committed rGbcfa7baec8bb: [mlir][CAPI] Add CAPI bindings for the sparse_tensor dialect. (authored by stellaraccident).
[mlir][CAPI] Add CAPI bindings for the sparse_tensor dialect.
May 10 2021, 9:55 AM
stellaraccident closed D102141: [mlir][CAPI] Add CAPI bindings for the sparse_tensor dialect..
May 10 2021, 9:55 AM · Restricted Project
stellaraccident added inline comments to D102141: [mlir][CAPI] Add CAPI bindings for the sparse_tensor dialect..
May 10 2021, 9:53 AM · Restricted Project

May 9 2021

stellaraccident accepted D102075: [mir][Python][linalg] Support OpDSL extensions in C++..

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.

May 9 2021, 11:57 PM · Restricted Project
stellaraccident added inline comments to D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding..
May 9 2021, 11:30 PM · Restricted Project
stellaraccident updated the diff for D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding..

Comments.

May 9 2021, 11:30 PM · Restricted Project
stellaraccident updated the diff for D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding..

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

May 9 2021, 11:26 PM · Restricted Project
stellaraccident added reviewers for D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding.: jdd, mikeurbach, ftynse.
May 9 2021, 6:16 PM · Restricted Project
stellaraccident requested review of D102144: [mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding..
May 9 2021, 6:15 PM · Restricted Project
stellaraccident added a reviewer for D102141: [mlir][CAPI] Add CAPI bindings for the sparse_tensor dialect.: ftynse.

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

May 9 2021, 4:19 PM · Restricted Project
stellaraccident requested review of D102141: [mlir][CAPI] Add CAPI bindings for the sparse_tensor dialect..
May 9 2021, 4:17 PM · Restricted Project

May 7 2021

stellaraccident added a comment to D102055: [mlir][linalg] Remove redundant indexOp builder..

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.

May 7 2021, 11:50 PM · Restricted Project

May 3 2021

stellaraccident committed rG9f3f6d7bd81e: Move MLIR python sources to mlir/python. (authored by stellaraccident).
Move MLIR python sources to mlir/python.
May 3 2021, 11:37 AM
stellaraccident closed D101493: Move MLIR python sources to mlir/python..
May 3 2021, 11:37 AM · Restricted Project
stellaraccident added a comment to D101493: Move MLIR python sources to mlir/python..

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

May 3 2021, 10:31 AM · Restricted Project
stellaraccident added a comment to D101493: Move MLIR python sources to mlir/python..

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.

May 3 2021, 10:31 AM · Restricted Project
stellaraccident added a comment to D101493: Move MLIR python sources to mlir/python..

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

May 3 2021, 10:31 AM · Restricted Project
stellaraccident updated the diff for D101493: Move MLIR python sources to mlir/python..

Address comments and rebase.

May 3 2021, 10:30 AM · Restricted Project
stellaraccident committed rGb57d6fe42ed3: [mlir][Python] Add casting constructor to Type and Attribute. (authored by stellaraccident).
[mlir][Python] Add casting constructor to Type and Attribute.
May 3 2021, 10:12 AM
stellaraccident closed D101734: [mlir][Python] Add casting constructor to Type and Attribute..
May 3 2021, 10:12 AM · Restricted Project
stellaraccident updated the diff for D101734: [mlir][Python] Add casting constructor to Type and Attribute..

Rebase

May 3 2021, 10:12 AM · Restricted Project

May 2 2021

stellaraccident added reviewers for D101734: [mlir][Python] Add casting constructor to Type and Attribute.: mikeurbach, jdd.
May 2 2021, 3:17 PM · Restricted Project
stellaraccident requested review of D101734: [mlir][Python] Add casting constructor to Type and Attribute..
May 2 2021, 3:17 PM · Restricted Project

Apr 29 2021

stellaraccident added a comment to D101493: Move MLIR python sources to mlir/python..

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
Apr 29 2021, 2:59 PM · Restricted Project
stellaraccident added a comment to D101493: Move MLIR python sources to mlir/python..

Seems mechanical, LGTM on the principle.

Apr 29 2021, 2:42 PM · Restricted Project
stellaraccident updated the diff for D101493: Move MLIR python sources to mlir/python..

Rebase.

Apr 29 2021, 2:25 PM · Restricted Project
stellaraccident retitled D101493: Move MLIR python sources to mlir/python. from DRAFT: Move MLIR python sources to mlir/python. to Move MLIR python sources to mlir/python..
Apr 29 2021, 2:17 PM · Restricted Project
stellaraccident edited reviewers for D101493: Move MLIR python sources to mlir/python., added: ftynse, mehdi_amini; removed: herhut.
Apr 29 2021, 2:16 PM · Restricted Project
stellaraccident updated the diff for D101493: Move MLIR python sources to mlir/python..

Fix install.

Apr 29 2021, 2:15 PM · Restricted Project

Apr 28 2021

stellaraccident accepted D101422: [mlir][python] Add `destroy` method to PyOperation..
Apr 28 2021, 5:37 PM · Restricted Project
stellaraccident added a comment to D101422: [mlir][python] Add `destroy` method to PyOperation..

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

Apr 28 2021, 5:15 PM · Restricted Project
stellaraccident added a comment to D101422: [mlir][python] Add `destroy` method to PyOperation..

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.

Apr 28 2021, 5:10 PM · Restricted Project
stellaraccident added a comment to D101496: [mlir] Move PyConcreteType to header. NFC..

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.

Apr 28 2021, 4:54 PM · Restricted Project
stellaraccident added a comment to D101496: [mlir] Move PyConcreteType to header. NFC..

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?

Apr 28 2021, 4:53 PM · Restricted Project
stellaraccident accepted D101496: [mlir] Move PyConcreteType to header. NFC..

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.

Apr 28 2021, 4:13 PM · Restricted Project
stellaraccident requested review of D101493: Move MLIR python sources to mlir/python..
Apr 28 2021, 3:12 PM · Restricted Project
stellaraccident added inline comments to D101364: [mlir][Python][Linalg] Adding const, capture, and index support to the OpDSL..
Apr 28 2021, 12:16 PM · Restricted Project

Apr 27 2021

stellaraccident accepted D101364: [mlir][Python][Linalg] Adding const, capture, and index support to the OpDSL..

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.

Apr 27 2021, 9:22 PM · Restricted Project
stellaraccident added a comment to D101422: [mlir][python] Add `destroy` method to PyOperation..

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?

Apr 27 2021, 8:25 PM · Restricted Project