- User Since
- Jan 6 2015, 1:11 PM (298 w, 2 d)
Mon, Sep 21
Removing errant changes and formatting
Fri, Sep 18
Nice. Just a few nits.
Thanks! It looks good to me, but I am not super familiar with the registration stuff, but looks fine overall.
Looks good after comment is addressed.
Looks good to me for a first draft.
Thu, Sep 17
Adding some comments. Looking through this right now.
Fri, Sep 11
Thu, Sep 10
Adding test for backward slice of Linalg operation.
Wed, Sep 9
@burmako This makes sense. I suspected this is what you were planning for, but as I said, it is not specific to this patch. I understand why you need this.
Tue, Sep 8
The specific IREEs use case might not be very relevant here (though this is part of this PR). Without going into those details, I think the expectation is that any user of Linalg tiling must be able to just use the LinalgTilingPattern. In IREE, I have a pattern class that inherits from this class. After tile (and distribute), I need the original operation to generate some IREE specific information, but the op is deleted. The alternative would be that I copy the guts of the matchAndRewrite method of LinalgBaseTilingPattern and use that directly, so that I can generate the information I need from the op before deleting it. I would prefer not to do that. My thought here is that this option is an opt-in for use cases where the original op is still needed after the tiling transformation. So a flag that allows you to opt-in to this made sense to me.
Wrong patch sent for review.
Not specific to the patch itself, but I am really concerned about the ballooning surface area of named ops in Linalg. I mentioned it before with the autogeneration from TC language approach to begin with, that one of the great things about Linalg is that many of the ops from dialects like MHLO (which have more justification for wide surface area of ops) can be boiled down to a "few" ops in Linalg. Adding more such operations here seems to be going away from that really useful feature. For example in this patch itself
Thu, Sep 3
Is there a way to add tests for these (do we need to)? Some roundtrip tests will serve as good reference.
Wed, Sep 2
Tue, Sep 1
Thanks! Can we also add some tests for the operation here and serialization roundtrip tests here. This is in keeping with the procedure to add new op here
Thu, Aug 27
Latest patch works, but I still need the fixes to the builds (actually the fixes to GPUTransforms/CMakeLists.txt above is for the patch earlier in the stack).
Looks good. Few minor comments. Please address before submitting.
Could we start a discourse post on this. Its more easy to document issues and discuss solutions there rather than on the patch.
Trying this change out, I needed the following changes to get this to build locally. Can you verify this is needed?
Wed, Aug 26
Looking at the stack here, I think only the change below this one in the stack (which is good to go) and this would probably need me to submit this. It would be good to commit the other two dependent changes and then I could try these two out locally.
This looks good! I just have a few final nits.
@limo1996 This is really nice find! I have been struggling with figuring out why convolutions are giving me errors in some cases, and I suspected something like, but wasnt able to nail this down.
Tue, Aug 25
Thanks for the edits!
Looks good to me, but maybe wait for @aartbik to review.
Thanks George! Please fix the clang-tidy errors before submitting if possible.
Aug 25 2020
Making this request changes. I think after addressing comments this is ready to land.
Aug 24 2020
I am not totally opposed to it though. Its one way of making the name optional, but still using the symbol table traits.
Aug 21 2020
Aug 20 2020
Aug 18 2020
Addressing comments and rebase.
Aug 17 2020
Thanks George! Just some minor comments.
I did an initial pass on this. Will come back and look in more detail, but have some high-level comments.
Aug 13 2020
Updating the patch with some changes to make sure that the allocation size bound computation works with addition of the affine.min operation
Aug 11 2020
Aug 10 2020
Removing errant tabs
Using affine maps for bounds/step update.
Aug 7 2020
Will commit this. THanks!
Aug 6 2020
THanks! Looks good to me!