User Details
- User Since
- Jun 18 2016, 2:24 PM (354 w, 6 h)
Thu, Mar 30
Looks good to me, nice!
Tue, Mar 28
Can you please reupload the full diff? The newest diff just looks like formatting changes.
Mon, Mar 27
Sat, Mar 25
Tue, Mar 21
Still, LGTM (feel free to land).
If we can't guarantee it can be generated, I don't think this should be added.
Thu, Mar 16
Wed, Mar 15
Patch looks reasonable to me.
I think for this revision we definitely need some high level usage docs
Mon, Mar 13
Took an initial scan, brain too fried to look at tablegen backend code but will take a look tomorrow.
Sun, Mar 12
LGTM with Jacques comments addressed
Fri, Mar 10
Thu, Mar 9
Wed, Mar 8
Tue, Mar 7
Is there a commit in this stack that handles updating/adding documentation?
Mon, Mar 6
Looking very close, thanks!
This patch seems to be doing a lot of things at once. I would split out registration in mlir-opt from the various IRDL things being added, and make things a bit more incremental, given the IRDL constructs could be tested via a test-pass (for example).
Fri, Mar 3
Why can't we just use the ReversePostOrderTraversal infra in LLVM?
LGTM
The failures look real, seems missing clang-format
Feel free to rename the API, I don't think the API churn in this case would be that bad.
I'd be fine landing this patch for now to fix the issue. The root of the inliner issues should be fixed when I rebase+land https://reviews.llvm.org/D134480
Thu, Mar 2
Feb 27 2023
Sure, can you provide an author name and email to attach to the commit? (arc doesn't want to patch this commit automatically for me).
Any idea of how this will affect build times? This code is now going to get instantiated a lot.
After looking at the followup, I'd honestly just fold this into the followup.
This patch feels kind of weird here, why not delay this until when it can actually be used?
Feb 25 2023
Feb 22 2023
Something that doesn't seem great about this approach is the need for every operation of every dialect to have an explicit entry. For MLIR that severely limits the situations that the grammar can be useful (effectively no downstream user can benefit), and is also hard to maintain, is there not a more generic fuzzy approach that gets us the correct highlighting most of the time (similarly to the textmate grammar)? @jpienaar
I am out, but this seems alright to me.
This seems like a fine start. It'd be nice if the name was a bit closer to "ThreadSafe" aspect, like "LockedSymbolTableCollection"(or something), to make it more apparent what the purpose is on first glance.
In addition to the stuff mentioned, I'd also love to see top-level docs detailing versioning, how it's structured, and how to hook in.
The windows build failures look real
Feb 17 2023
Haven't had time to dig in here, but adding an attribute doesn't feel right for version. Why is this necessary? As opposed to being something specific to the assembly format?
Feb 16 2023
How does the interact with dialect conversion?
Feb 14 2023
Nice