- User Since
- Aug 20 2014, 6:06 PM (322 w, 1 d)
This seems to have broken the build https://buildkite.com/mlir/mlir-core/builds/8787#c1ee666b-58ff-4f6e-816e-2ba4996fa2ee
Wed, Oct 21
Tue, Oct 20
Looks good, thanks! An extra failure case for line 1189 would be nice to have.
Mon, Oct 19
Could you please update the documentation and description of the change?
Looks good - this is little bit of a big review, but I also don't have an idea of how to make it smaller easily. Given the work following on this & verification wrt existing, I feel a little bit more comfortable.
Fri, Oct 16
Nice, micro nit: in description add ... as when I initially read it I thought there was requirement for empty block bb1
Nice, to follow the naming convention of other utils folders here, how about mlir/utils/vscode/mlir.natvis ? (or s/vscode/vstudio/? I don't now who which is considered more general/common in folks minds :)).
Woops, I missed this asking for this test. Thanks
Thu, Oct 15
Looks good thanks!
Nice - well I think this means I can stop migrating the other OpBuilder usage as these will be migrated again :)
Wed, Oct 14
Looks good overall
Tue, Oct 13
Looks good, thanks
Mon, Oct 12
I think you may need to rebase this on head, seems like you were working of an old version here.
Fri, Oct 9
Thu, Oct 8
Wed, Oct 7
The pre-merge differs here - if you want to appease both you'd probably need to use PRIu64 (https://en.cppreference.com/w/c/types/integer) it seems
Tue, Oct 6
Mon, Oct 5
Sat, Oct 3
Addressed typos and used more similar naming to ScopedPrinter
Fri, Oct 2
This seems to have broken GCC 5 (https://buildkite.com/mlir/mlir-core/builds/8301#e6b6e5a7-85bd-4454-ae2b-da3b6cf84b8a). Could you update? Or should we revert?
First partial scan (sorry forgot to hit send on this earlier)
Wed, Sep 30
Tue, Sep 29
Thanks for doing this!
Mon, Sep 28
There is also just returning an error too (it's not just a fatal error or
assert dichotomy here).
Sat, Sep 26
Wed, Sep 23
Sep 22 2020
Indeed, thanks this is super nice (I missed the ping here but saw the commit on GitHub and pointed folks to it)
Thanks, LG with Rahul's comments addressed.
Sep 21 2020
LG, modulo I think the error discussion on the shape_eq one wasn't updated there or used here. But we can do those as uniform follow up too.
Lets just focus on "Change the default builders to use TypeRange instead of ArrayRef<Type>" part as one change rather than having 2 unrelated changes in one. That part LGTM to, the other I'd prefer a follow up discussion.
Sep 19 2020
Sep 17 2020
Thanks, I am tempted to suggest we add "use this only if more specific form is not available" (e.g., use this for cases not expressible with the others). So that we push to simpler forms.
Sep 15 2020
Nice, looks like good reduction
Sep 11 2020
The SameOperandsAndResultElementType is redundant with the new
TypesMatchWith and prevented having zero elements.
Sep 5 2020
Sep 4 2020
Sep 1 2020
Aug 31 2020
Aug 28 2020
Looks good, thanks
Aug 27 2020
Aug 26 2020
Aug 22 2020
Nice, thanks. Could you add a test with an optional attr that would have failed here before?
Aug 20 2020
Nice thanks, LG with comments addressed
Aug 18 2020
Aug 17 2020
Aug 15 2020
LGTM if towards removing static registration