- User Since
- Dec 20 2019, 9:16 PM (78 w, 1 h)
I've got some pretty strong evidence that this is indeed deadlocking during verification processing, but I can't quite explain why.
Wed, Jun 16
Thanks. Rename lgtm
Tue, Jun 15
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.
Mon, Jun 14
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.
Fri, Jun 11
Thu, Jun 10
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.
Fri, Jun 4
Can we please have an explanation/rationale for reverts? Given the short time window, I'm left to assume something was done in error.
Thu, Jun 3
Tue, May 25
Thank you for the fix!
Mon, May 24
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.
Sun, May 23
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.
Nice improvement - thanks. Do you need me to land this for you?
Sat, May 22
May 19 2021
Thank you - not having this was unintentional.
May 16 2021
May 15 2021
Thanks for the patch! I'm accepting this to help with timezones but would like my comments addressed/further discussed before landing.
May 12 2021
May 10 2021
This broke the windows build bot, and I missed it. Rolling forward fix in: https://reviews.llvm.org/D102189
Addressed comments. Thanks.
Rebase and comments.
May 9 2021
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.
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.
May 7 2021
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 3 2021
Alex: Made a change that might appease your machine. Let me know.
Address comments and rebase.
May 2 2021
Apr 29 2021
Apr 28 2021
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 27 2021
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?