Comments addressed as https://github.com/llvm/llvm-project/commit/63779fb462d828d16b87f427a6490dded842ca15

Oops. Pushed an older commit without squashing fixes...

Thanks Denis!

Add file to CMakeLists.txt

Nice clean up!

In D74143#1884098, @mehdi_amini wrote:

In D74143#1881674, @antiagainst wrote:

To truly allow dialect writers to dictate what naming style they want to use, the dialect writer need to control what naming convention is used to generate these attribute getter methods too.

I suspect any attempt to provide such facility (switching naming convention) will hit the issue of mismatch with Traits and inherited properties: I think it will require a major change in our infrastructure to provide this.

Don't forget to update the CL message too.

Nice! Thanks!

Nice!

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.

Awesome! Thanks again, Denis! This is very helpful!!

LGTM!

Nice, thanks!

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?

LGTM, but may want another pair of eyes to check the Linalg specific logic. :)

In D73826#1873559, @mehdi_amini wrote:

In D73826#1865623, @antiagainst wrote:

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

There is no disagreement that this may be useful for some cases: but we should strive to improve ODS itself to provide this facilities. I don't think we want dialect author to write Tablegen backends: this defeats the whole point of ODS in my opinion!

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.

In D72696#1874164, @denis13 wrote:

Thanks a lot for review! I addressed comments related to pass in separate revision https://reviews.llvm.org/D74549 and will update mlir-vulkan-runner, VulkanRuntime as well.

Nice, thanks Denis! Reviews already happened in D72696 so I just have two more nits here.

Nice! Thanks River for adding support for this!

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. :)

Add comments on FunctionLike

Address comments

In D72533#1865938, @bondhugula wrote:

For the naming part, could we consider using IntegerType for signless integers the way it is now and add SignedIntegerType? i.e., see "Signed" as an addition as opposed to an adjective/specialization, and continuing to maintain that integer types in MLIR are signless.

A couple of things.

1. The commit message has no mention of 'SignedIntegerType' or 'SignlessIntegerType'. Please add something to the effect "add 'SignedIntegerType' and IntegerType -> SignlessIntegerType".
2. The last message on https://groups.google.com/a/tensorflow.org/d/msg/mlir/XmkV8HOPWpo/7O4X0Nb_AQAJ was in Jul 2019. Could you mention the naming change on the discussion forum?

• Fix Rationale wording

In D72696#1867975, @denis13 wrote:

@antiagainst @mravishankar this is an initial version of mlir-vulkan-runner. It's now possible to execute GPU code on Vulkan capable devices via JitRunner.

@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.

Address comments

Nice, thanks!

Address comments

• 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.

LGTM.

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

In D74012#1858709, @mravishankar wrote:

I understand what the intent is here, but the input already has an attribute that belongs to the SPIR-V dialect before lowering.

LGTM, just two nits regarding the test names.

Nice!

Remove \n

Update doc

In D73579#1852960, @mehdi_amini wrote:

@antiagainst : what is the best way to test that?