In D147262#4235296, @devajithvs wrote:

Thanks @rriddle, done!
Am I correct in assuming that struct names remain in the same format (snake_case)?

Looks good to me, nice!

Can you please reupload the full diff? The newest diff just looks like formatting changes.

In D145582#4210683, @gysit wrote:

In D145582#4210671, @rriddle wrote:

Still, LGTM (feel free to land).

I was waiting due to the blocking review of @jpienaar. Is it ok to land nevertheless?

Still, LGTM (feel free to land).

If we can't guarantee it can be generated, I don't think this should be added.

Patch looks reasonable to me.

I think for this revision we definitely need some high level usage docs

In D145991#4191347, @mehdi_amini wrote:

This assumes less threads than shards overall right?

Took an initial scan, brain too fried to look at tablegen backend code but will take a look tomorrow.

LGTM with Jacques comments addressed

In D145066#4168413, @mehdi_amini wrote:

In D145066#4167514, @rriddle wrote:

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

Will this be enough to solve the entire dump including the members of the InlinerPass class?

Is there a commit in this stack that handles updating/adding documentation?

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

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

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?

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

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?

How does the interact with dialect conversion?

Nice