This looks fine to me. Thanks for adding this.
But please add serialization tests as well

Might be worth checking that the serialization/deserialization works as well

Change to test to check for scalar load

Addressing comments and fixing failing test

Removing errant ;

Overall looks fine to me.

Thanks Stephan! I see overall where this is headed. This is nice to have indeed.

THanks Lei! More I think about it, it is better to use this approach.

Maybe I am missing something here, but from the GPU dialect, the sizes are passed to the gpu.launch, so you can take them from there. If you want to specialize a kernel for specific sizes, you need to ensure compatible call sites, like in other function specialization. Is this more about driving upper layers of code generation so that you end up with a gpu.launch that has sizes you want? Or do you want to make gpu.func usable independent of the gpu.launch?

Thanks Lei! Accepting conditioned on addressing other comments.

Addressing missed comment.

Addressing comments

THanks for taking this one now. Its already touching so much code, if we left it for longer it would be more effort to use spv.function

(Sorry for jumping on this late)

Updating after addressing comments

Addressing some clang format issues

Actually updating the file to fix build errors.

THanks for adding the cosine tests too...

Updating with changes to fix build failures

Thanks Natasha! This needs tests though.
See mlir/test/Dialect/SPIRV/glslops.mlir for similar ops. These are round-trip tests. It checks that mlir can parse the operation and print it out correctly
You also need "Serialization" tests, i.e. MLIR can generate valid SPIR-V binary for this operation. See mlir/test/Dialect/SPIRV/Serialization/glsl-ops.mlir

Lets go with this for now. We can clean this up when the attribute story gets fixed up.

I understand what the intent is here, but the input already has an attribute that belongs to the SPIR-V dialect before lowering. That makes things a bit non-composable. In cases where someone lowers to the GPU dialect and then conditionally decides to lower to SPIR-V dialect or the NVVM dialect, with this change on the SPIR-V side a separate pass will be needed to add this attribute. Ideally the input should be only in GPU dialect, whereas here it isnt.
Is it possible instead to add an attribute to GPU dialect itself which contains information about the workgroup size. Then while lowering we can convert one attribute to another.

Thanks!

The (de)serialization needs to be implemented. Will that come later?

THanks Denis. Looks great!

Thanks Denis for taking these up and driving them

Could you add some serialization tests as well. Thanks

Could you add some serialization tests as well?

Adding link to chunked Vulkan Spec pages.

Addressing comments from antiagainst

Thanks!

@nicolasvasilache : Totally agree with all the points you make. I understand the advantage of performing fusion at the tensor level. I was under the impression that there would be a separate dialect for "Linalg operations on tensors", and the buffer allocation pass would "convert" from that dialect into the existing Linalg dialect. But, I guess inter-mixing of operations from different dialects to having different "incarnations" of the operation (one operating on tensors and other on buffers) is equivalent. I see that this makes sense for generic ops, but how would this work for other operations like linalg.matmul and linalg.matvec, etc. where I am not sure you can have a tensor argument. So it seems like this is making generic ops different than other linalg ops.
Another question is about buffer allocation. Is there some buffer allocation already implemented in Linalg?
In any case, since the contract between Linalg and other "backend" dialects is that the lowering is only supported if Linalg ops work on memrefs, I dont really have any major concerns and happy to see how things work out.

This is awesome! Fantastic work!

Thanks for fixing the naming!

Without commenting on the specifics of the change itself, was wondering what advantage there is to allow generic ops to take tensor values as well. Until now lowering from Linalg to (say) GPU Dialect was fairly straight-forward cause Linalg operated on memref types. I am not sure what would happen if the generic op with tensors would be lowered to GPU dialect (presumably should be an error), and lowering it correctly would need some buffer allocation to kick in. So it seems like this is adding complexity. Could you please provide some context behind the need for this change?

SPIR-V parts look good to me. Would still prefer the version/capability related methods into a separate file. They seem to have disjoint functionality (but its a personal preference).

Just wondering why the refactoring?

The SPIR-V part looks fine to me. I have no major comments on the tblgen part. Maybe get some one with more knowledge to review it.

Thanks. Looks good.