- User Since
- Dec 24 2019, 5:47 AM (9 w, 2 d)
Comments addressed as https://github.com/llvm/llvm-project/commit/63779fb462d828d16b87f427a6490dded842ca15
Oops. Pushed an older commit without squashing fixes...
Add file to CMakeLists.txt
Mon, Feb 24
Nice clean up!
Fri, Feb 21
Don't forget to update the CL message too.
Cool! Thanks River for adding this! Can we also update the doc ?
Implementation looks good to me.
Nice! Can we also update the doc regarding which traits are supported? Otherwise it's quite obscure for somebody to figure out what works and what not.
Wed, Feb 19
Awesome! Thanks again, Denis! This is very helpful!!
Tue, Feb 18
That seems independent of this though, or are you thinking the variants of attribute methods? E.g., we'll have multiple different convenience accessors generated for the same attribute beyond the raw and convenience one we have today?
Thu, Feb 13
LGTM, but may want another pair of eyes to check the Linalg specific logic. :)
I'm concerned about the method name here. Although we can rely on compiler symbol resolution, it's still quite confusing to see two attributes with the same name but one for getters and one for setters. If we also think about the getter method returning "unwrapped" values, that's three.. So if I have a some_stuff attribute, we have some_stuff(), some_stuffAttr(), and some_stuffAttr(some-params). It's also quite sad to see the mix of different naming conventions here. And I think the number of attribute methods is just gonna to grow. It would be far better if we have getSomeStuff(), getSomeStuffAttr(), and setSomeStuff(). FWIW, I still think having proper naming convention and method prefix/postfix control at dialect level is the better way to go given that we don't mandate how attributes should be named for ops.
Nice, thanks Denis! Reviews already happened in D72696 so I just have two more nits here.
Nice! Thanks River for adding support for this!
Wed, Feb 12
Super nice! This is so great! Thanks a ton, Denis! I don't have major concerns given that a large portion of this patch has already went through reviews multiple times previously. Just a few nits. We can land and then iterate on it. :)
Tue, Feb 11
Add comments on FunctionLike
- Fix Rationale wording
Mon, Feb 10
@herhut: +1 to what Mahesh said. Additionally, I'd like to tighten SPIR-V side to use attributes in general for passing in pattern configurations.
I would actually prefer that we call this FuncOp('func'). It is consistent with all of the other functions that have been defined, and seems a little weird to deviate. For example, spirv::ModuleOp/ConstantOp are also named the same as other common ops.
Sat, Feb 8
- Went through isa<IntergerType> and isInterger(...) use sites to make sure they now use signless integer type where possible.
- Updated several APIs to include 'Signless' in the name to be explicit.
- Fixed typos
The main value I see with the OpDefinitionGen hook is that it allows one to automatically insert traits and methods into the generated C++ op classes. This is preferable when a dialect's ops have a large variation of potential traits and methods, and that information is already encoded with other fields in the op definitions.
It seems the parsers/printers modified in this patch does not use the attribute-type-ellison feature introduced in this patch? Can we touch parsers/printers really uses this feature? Also should we document this? Otherwise it's subtle behavior users might find very confusing. (The modifications to parsers/printers in this patch can be split out.)
Please also fix the test.
Fri, Feb 7
Wed, Feb 5
Cool stuff! :) This reminds me that we don't have proper debug information to and from SPIR-V binary format too. Need to add that some day too. ;-P
LGTM, just two nits regarding the test names.
Tue, Feb 4