Do we need a unit test at all here? If I understand the patch correctly, you're saying that the canonicalize options aren't being copied over correctly when a pass is cloned. If that is true, then we can exercise this with a standard filecheck test of the canonicalize pass.

Whoa, great catch. Only request: please merge the unit test into the existing test/lib/Pass stuff. Thank you!

I'm not a competent reviewer for this patch.

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.

Oh, I completely missed that. I would recommend using the word case_insensitive here. I don't think stringref has any other case insensitive support, and I don't think this is the obvious place to start. I would start with case insensitive equality comparison. Your client can use that as a primitive that it builds higher level functionality on top of.

I was thinking that the closure would take a character, not a stringref. So you could do something like str.consume_back(islower).

I'd prefer it if someone else could review this code. It has been a very long time since I've touched it.

I don't think this is general enough requirement to add a specialized method for it. Did you consider adding a consumes_front/back overload that takes a lambda?

Clean up the comments.

Remove the changes to Pass.cpp, they were supposed to be in a follow-on

Incorporate River's feedback.

Thank you for the quick review!

Yes, thank you! Too many similar things.

The new patch is available here: https://reviews.llvm.org/D103373/new/

I filed https://bugs.llvm.org/show_bug.cgi?id=50701 to track cases where the PassManager is verifying things it shouldn't IMO. Fixing that will be an orthogonal improvement to this patch.

Thanks!

nice, thank you!

I don't know how to revert this, but feel free if you see it as a smoking gun. I only abandoned this because "it didn't show a big speedup and I have a hopefully better idea of how to improve things", not because I think this is harmful

LGTM, thanks!

I'm not sure how this landed, it looks like it got pushed as a consequence of the twine patch. Maybe I put them both into the same directory accidentally (my git fu is not great).

spell out auto

tweak commit message.

Simplify the API for Identifier::get and StringAttr::get to always take Twines.

I'm open to that, only two downsides I can think of:

1. absolutely tiny additional runtime overhead, slight codebloat for making the temporary twine. I'm not concerned about this.

xref: https://github.com/llvm/circt/pull/1202#pullrequestreview-676800939

I'm abandoning this patch. With the other general improvements to dominators and with the inefficiency in the threading stuff, this is no longer a win.

update commit message for real this time.

Update commit message

Ok, I think this patch is ready for a re-check. Thanks!

Final grooming, remove commented out #include, use range base for loop again.

Dramatically simplify this patch by merging in the dominator improvements.

Self approving as 'obvious'

This required adding quite a few additional includes, as many files were relying on various things pulled in by ArrayRef.h.

This got a lot of valuable feedback already, I'm going to land this to hill climb on top of it (e.g. https://reviews.llvm.org/D103373). I value any other input of course!

Use setOrderIDForThread correctly.

Updates from River's review

SGTM, I'll revise this after the other patch lands, thank you for the review!

Incorporate feedback from review.

thanks for all the feedback, incorporated!

Thank you so much for tackling this!

The dominators rewrite is here: https://reviews.llvm.org/D103384. Once that goes into tree I'll merge it into this patch to simplify it - we can drop the "don't recurse into IsolatedFromAbove regions" change clearly and the manual laziness isn't needed, but the parallel constructs are.

Adding a few others who have touched this code.

Actually, instead of making this more complicated, I'd like to keep this simple in this patch as it is.

Use isBeforeInBlock instead of maintaining a SmallPtrSet.

Improve comments.

Self approving as "obvious"

Self approving as "obvious"

This is really great River, thank you and nice work!

Improvements suggested by River

Thank you for the quick review!

Move emptyString and rename it to be more similar to other cached attrs.

Good call, thanks for the review!

Fixed in a6a57f03be40, thanks for the head's up!

taking a look