User Details
- User Since
- Jun 28 2016, 7:50 PM (351 w, 2 d)
Thu, Mar 16
this motivation makes sense to me, but I'm not a competent reviewer for this area
Mon, Mar 13
Super awesome, thank you again for driving this!
Sun, Mar 12
Amazing, thank you for tackling this!
Fri, Mar 3
Nice, thank you!
Thu, Mar 2
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.
Mon, Feb 27
Cool didn't know about enable_if = delete, TIL thank you for tackling this!
Feb 19 2023
Seems reasonable to me
Feb 15 2023
Awesome, thank you for driving this!
Feb 14 2023
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 :)
Feb 13 2023
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.
Feb 10 2023
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.
Feb 8 2023
I'm sorry, but accepting this patch will be very difficult for the project. I have several technical concerns with this patch:
- 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.
- This seems to overlap a lot with the llvm libc work, particularly for the simple numeric operations.
- This appears to add code with new dependencies, e.g. gmpfrxx? and has internal abstractions that aren't necessary for libcxx
Feb 6 2023
I'm sorry but I'm too far out of this code to review it.
Feb 1 2023
Looks fine with the requested changes.
Jan 27 2023
Jan 19 2023
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
Jan 15 2023
Thank you for the review!
Jan 14 2023
Jan 12 2023
Jan 9 2023
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!
Dec 6 2022
Nice!
Dec 3 2022
nice Joe!
Dec 2 2022
nice, thank you for this!
Nov 25 2022
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!
Nov 14 2022
Nice! I'd love someone else's eyes on this, but that's a great speedup!
Nov 11 2022
This LGTM and makes sense. Nice use of the alignment apis.
Nov 2 2022
I do need the llvm:: namespace qualifiers unfortunately; a lot of other functions in this file use them too. :(
Nov 1 2022
Very exciting Lily, thank you for implementing this!
Oct 28 2022
I have no objection to removing these!
Oct 25 2022
Seems fine to me, but I don't work on the C API so I can't advise on it.
Oct 24 2022
This is super exciting River! Baby MLIR is growing up to be a real compiler! :)
Oct 20 2022
+1 for revising the name "pure" :-)
Oct 15 2022
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.
Oct 14 2022
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!
Oct 13 2022
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.
Oct 12 2022
Nice Nick, +1 for updating in tree uses if there are any
Oct 1 2022
Thank you, this is very helpful!
Sep 28 2022
Nice, thank you for the test
Sep 27 2022
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!
Sep 20 2022
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.
Sep 17 2022
Very cool, it'd be great to improve this in tblgen!
Sep 14 2022
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!
Sep 9 2022
nice, thanks Joe!
Makes sense!
Sep 7 2022
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.
Aug 31 2022
Adding a few reviewers who have worked on this, Russell Gallop has worked on something directly related.
Aug 30 2022
Awesome, thank you for improving this!
Aug 26 2022
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.
Aug 25 2022
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
Aug 23 2022
This LGTM, thank you for the ping!
Aug 22 2022
+1, thanks for clarifying this. I recently hit this as well.
Aug 18 2022
LGTM with tanya's suggestions!
Aug 17 2022
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.
Aug 15 2022
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!
Aug 11 2022
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:
Aug 10 2022
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!
Aug 9 2022
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?
Aug 8 2022
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:
Aug 7 2022
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?
Aug 6 2022
+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 :-)