this motivation makes sense to me, but I'm not a competent reviewer for this area

Super awesome, thank you again for driving this!

Amazing, thank you for tackling this!

Nice, thank you!

Cool. Suggestion: instead of duplicating the APIs, you could add a , bool isKnownNullTerminated = false flag. That keeps the API simpler and maintains a safe default while allowing clients to avoid the copy if they are sufficiently smart and already null terminated.

Cool didn't know about enable_if = delete, TIL thank you for tackling this!

Seems reasonable to me

Awesome, thank you for driving this!

We generally don't worry to much about compatibility issues. It is good to deprecate the old name for a few weeks before removing it, but we don't do long cycles here.

Bikeshedding is good @foad, these are important core data structures :)

Very cool, I'm excited to get this. This will simplify a lot of code! Could you also add this to StringMap? It has a similar lookup method but it can't be used for non-default-constructible values, and has the same usage pattern as this container.

I'm not familiar with the std::from_chars example, but GoogleBenchmarks is not part of the "shipped" libc++ product, it is an internal implementation detail.

This makes sense to me, thank you Fabian. That said, please also wait for another person to take a look at this. Some folks have opposed expanding the API of mlir-translate before and I'm not sure exactly what the boundaries we want are.

I'm sorry, but accepting this patch will be very difficult for the project. I have several technical concerns with this patch:

1. This is a very large block of code which does not follow LLVM conventions and has a lot of boost support functionality that seems redundant with stuff elsewhere in LLVM.
2. This seems to overlap a lot with the llvm libc work, particularly for the simple numeric operations.
3. This appears to add code with new dependencies, e.g. gmpfrxx? and has internal abstractions that aren't necessary for libcxx

I'm sorry but I'm too far out of this code to review it.

Looks fine with the requested changes.

Got it this time, sorry for the confusion!

I'm pretty sure I'm on top of commit access requests now, plz let me know if I missed you or something! Could be spam filter or who knows what

Thank you for the review!

Nice, thank you for doing this!

Wow, great catch, thank you for fixing this! SourceMgr is used by more than just tblgen btw, so fixing this is great. 161 seconds to 6 seconds is quite the speedup too :-)

This is so great Jeff, thank you for driving this forward, I love the boilerplate destruction!

Nice!

nice Joe!

nice, thank you for this!

Nice, seems overdue. Tiger (10.4.10) was the first release to ship LLVM as part of the OS, so special memories there :-). Thanks for cleaning this up!

Nice! I'd love someone else's eyes on this, but that's a great speedup!

This LGTM and makes sense. Nice use of the alignment apis.

I do need the llvm:: namespace qualifiers unfortunately; a lot of other functions in this file use them too. :(

Very exciting Lily, thank you for implementing this!

I have no objection to removing these!

Seems fine to me, but I don't work on the C API so I can't advise on it.

This is super exciting River! Baby MLIR is growing up to be a real compiler! :)

+1 for revising the name "pure" :-)

More information on the GCC/Clang attribute semantics:
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

I just noticed this patch when it got merged into our tree.

This will remove so much boilerplate, thank you!

agreed, I'm not in love with the existing API, but I don't want to break everyone for no reason. Thanks for the review!

This is an obvious patch and tested with out of tree clients. There isn't an obvious owner for this file at this time besides me :-/, anyone object to landing this?

awesome, thank you!

Awesome!! Please adopt this in a few places just to exercise the new logic. a wholesale swapover can be done in a separate patch, but it is good to show that the new syntax works.

Nice Nick, +1 for updating in tree uses if there are any

Thank you, this is very helpful!

Nice, thank you for the test

Is there a reason to have two names for the same thing? Why shouldn't the omp dialect be moved into a directory "omp"?

+1 this makes sense to me, but I'd love for someone else's eyes to be on the details. Thank you!

Thank you for clarifying this!

The problem that I ran into was that a lot of stuff kind of requires that the Inits get folded and are uniqued, which would no longer be the case if we put locations on them. From there this patch arose, because I didn't want to broach the "rewrite the world" just to get better locations.

Very cool, it'd be great to improve this in tblgen!

Do you have a sense of what (in practice) is causing the dead operations to get emitted? Can they practically be handled upstream? Such a thing would be a win for all the upstream cost models and would reduce compile time.

nice, thank you Jeff!

nice, thanks Joe!

Makes sense!

Thank you for improving this Uday! (and also tolerating the long and winding side discussion) :-) :-)

Yes, that's a fair point, I didn't think about StringAttr. This discussion is pretty far afield from the title on this PR - can we move it to its own issue?

I agree that it would be useful to have a "get as string" convenience next to these things generally. Please name it "getAsString()" though to follow the general naming conventions which prefer "get". LLVM is wildly inconsistent across subprojects, but MLIR is relatively good about following this convention.

fwiw, the reason that LLVM conventionally has the ->dump() methods is for use in debuggers. Modern debuggers still aren't able to resolve call thing->print(llvm::errs()) super reliably.

Adding a few reviewers who have worked on this, Russell Gallop has worked on something directly related.

Awesome, thank you for improving this!

This is really great Jeff! Thank you for cleaning this up!

All that code already exists in Diagnostic::appendOp, we need incremental ~+2 more lines on what is in main.

As mentioned above, this isn't enough and is just wrong. Some ops may print empty regions as {} on different lines.

The goal of this makes sense to me. That said, would it be possible to key this off of "whether the printed form of an op has \n's in it"?

It has been a very long time since I've worked in this code, but I think the bug is up-stream, whatever is causing the phi nodes to not be adjacent

This LGTM, thank you for the ping!

+1, thanks for clarifying this. I recently hit this as well.

LGTM with tanya's suggestions!

Nice, I'm excited to see a less monolithic approach to GEN_PASS_CLASSES, this will help for passes.td files with multiple passes that have differing dialect requirements etc. Right now you have to forward declare a lot just because they are used by other passes. very cool!

This is a great idea, thank you for doing this. I think we should also point out the sublicenses that are not llvm licensed, e.g. we have a copy of PCRE, googletest etc, and should notify people about these things.

Ok, I'm happy to see what Jeff has in mind.

I didn't review the patch in detail, but +1 this is a great step forward to reorganize the repo!

nice, thanks!

Awesome, thank you for pushing this forward. What do y'all think about changing the empty syntax to just array<i8>? It looks weird to have the colon there in the empty case.

These seems helpful to cut down on the number of "near variants" of the same custom format. Jeff, it might help to adopt this in tree. One random example I noticed are the ones in ViewLikeInterface:

awesome! thanks :)

Ok - are you planning to go with something like the current patch where you just add unsigend integers (I8 and UI8), or are you also adding explicitly signed things as well (I8, UI8 and SI8)?

I'm opposed to it, because it is unclear why it is valuable.
That is: it adds design complexity where someone in ODS now has to choose between multiple options without obvious discriminator. That leads to fragmentation and inconsistencies (the kind of complexity that contributed to plague DenseElementsAttr users).
I see the case of a "gep" operation or the slice @Mogball mentioned above as intrinsically different than a constant op. The constant op is just materializing an SSA value for the attribute, which justifies supporting MLIR type associated with the attribute somehow, but for the others it does not. It isn't clear to me why it is a good thing to unify the attribute for both cases, even they both store "an array" ultimately.

I haven't reviewed this in detail, but oh my I love LOVE LOVE this! llvm::scc_traversal(x) is so nice, great work!

nice Jeff!

nice, thank you for improving this!

Right, I think that use case can work without this, with less safety/checking that having this checked and enforced in ODS. Looking beyond the LLVM dialect, are you arguing that properly typed accessors aren't useful for any client?

It's required by ElementsAttr interface. Whether DenseArrayAttr needs to conform to that interface is up for discussion, but it does provide a lot of the machinery for querying the underlying values.

Nice, I love all the minuses!

Responding to a couple of questions, paraphrasing:

Out of curiosity, why does DenseArrayAttr conform to ShapedType? That seems really specific to multidimensional things, why not just have an array-like getSize() interface like ArrayAttr?

+1, thank you for not doing bit compression!

You're right that signedness doesn't affect the semantics of the bits in memory, but some clients care about signed semantics in their type system. I think both approaches are principled, but we should look to make MLIR consistent across the builtin storage attributes. For example, if IntegerAttr supports/maintains the i32/si32/ui32 distinction then it would make sense for DenseArrayAttr to support them as well. Alternatively, it would also be principled for nothing to support signs in the storage types: both designs work but we shouldn't do half and half :-)