Addressing comments

Thanks!

Removing errant changes and formatting

Nice. Just a few nits.

In D87781#2281741, @nicolasvasilache wrote:

Putting a blocker to get some discussion here.

The original definition was following TF, it is not anymore.
Do we consider this to not be a problem ?

Putting the loops in the "right" order is independent from the op spec.
You should be able to just use tiling without any op definition change (tile by 1-1-1-1-1 + perm for scalar loop-level).

More generally, the way I am approaching this longer term is that this magic "embed the world op" would disappear in favor of more, smaller named ops.
Transformations would look like: named_op1 -> generic form -> named_op2 and I think a lot of these things can be automated from the TC form.
My overall concern is whether we are going towards relying more and more on an op with very fat one-off semantics.

I won't oppose it strongly for now but the conv op will most likely be a prime candidate for deletion when the infra is more mature, so I'd be cautious on putting a lot of effort on transformations / optimizations / perf results if they don't generalize to linalg.generic.

Also, the doc would need to be updated too.

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.

Adding some comments. Looking through this right now.

In D87308#2262920, @nicolasvasilache wrote:

The alternative would be that I copy the guts of the matchAndRewrite method of LinalgBaseTilingPattern

Right so my suggestion here would be to make LinalgBaseTilingPattern to *never* erase the op and the existing derived LinalgTilingPattern override matchAndRewrite to call the base and then eraseOp.
Then your patterns can do the same: override matchAndRewrite, call the parent matchAndRewrite, do X and eraseOp.

It seems we can do that without adding a new option.
The reason I'd prefer to avoid new options is that any new knob is more complexity to precondition/search.

Addressing comments

rebase

Adding test for backward slice of Linalg operation.

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

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

Nice! THanks!
Is there a way to add tests for these (do we need to)? Some roundtrip tests will serve as good reference.

Thanks!

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

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

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

In D86108#2242654, @georgemitenkov wrote:

In D86108#2241210, @mravishankar wrote:

Trying this change out, I needed the following changes to get this to build locally. Can you verify this is needed?

index 560dcae8200..41f5c8a9ce5 100644
--- a/mlir/lib/Conversion/GPUCommon/CMakeLists.txt
+++ b/mlir/lib/Conversion/GPUCommon/CMakeLists.txt
@@ -34,6 +34,8 @@ add_mlir_conversion_library(MLIRGPUToGPURuntimeTransforms
   MLIRIR
   MLIRLLVMIR
   MLIRPass
+  MLIRSPIRV
   MLIRSupport
   MLIRStandardToLLVM
+  MLIRSPIRVToLLVM
 )
diff --git a/mlir/tools/mlir-spirv-cpu-runner/CMakeLists.txt b/mlir/tools/mlir-spirv-cpu-runner/CMakeLists.txt
index 69080ae66dc..8e0c2a6ad67 100644
--- a/mlir/tools/mlir-spirv-cpu-runner/CMakeLists.txt
+++ b/mlir/tools/mlir-spirv-cpu-runner/CMakeLists.txt
@@ -15,6 +15,7 @@ if (MLIR_SPIRV_CPU_RUNNER_ENABLED)
   get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 
   target_link_libraries(mlir-spirv-cpu-runner PRIVATE
+    LLVMLinker
     ${conversion_libs}
     ${dialect_libs}
     MLIRAnalysis

I am building locally without these changes and it is working for me?

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?

THanks again!

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.

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.

Making this request changes. I think after addressing comments this is ready to land.

I am not totally opposed to it though. Its one way of making the name optional, but still using the symbol table traits.

Addressing comments and rebase.

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.

Thanks George!

Updating the patch with some changes to make sure that the allocation size bound computation works with addition of the affine.min operation

Removing errant tabs

Using affine maps for bounds/step update.

Will commit this. THanks!

THanks! Looks good to me!