[Mlir-commits] [mlir] [mlir][TilingInterface] Use `LoopLikeOpInterface` in tiling using SCF to unify tiling with `scf.for` and `scf.forall`. (PR #77874)

[llvmlistbot at llvm.org]

MaheshRavishankar wrote:

> This is the followup from #72178, right? Let's mention it in the commit message so we can refer back to it later, had to do a bit of spelunking to find it.

It is a change from a long list of related changes, but yes, https://github.com/llvm/llvm-project/pull/72178 is probably the start of it. Updated the comment anyway.

> 
> Overall comment:
> 
> So IIRC, #72178 adopted the logic of tiling `scf.forall` from `LinalgTransformOps.cpp` and/or `Linalg/Transforms/Tiling.cpp` to redo `scf.for`.
> 
> This was a welcome refactoring and this PR is a nice continuation, nice!
> 
> To proceed confidently, I would now like to see red diffs in `LinalgTransformOps.cpp` and `Linalg/Transforms/Tiling.cpp` to see older implementations be actually replaced: atm this seems it is still adding more code in parallel to the old implementation.

Yes, that is the goal, but I want to proceed incrementally. With this change, the same tests that work with `scf.for` work with `scf.forall` (so interchange, scalars, etc. etc) and avoids duplicating code for tiling using the two loop constructs. The next step for me is to move over the tiling logic that exists currently in `Linalg/Tiling.cpp` and `LinalgTransformOps.cpp` to this and fix as needed.

There is lot of context here spread all over the place w.r.t tiling. I want to unify all of it, but I dont think I can do that in one step. I would rather incrementally build up the required functionality in one place and deprecate/move pieces as needed. I do plan to do this in short order though..

> 
> For instance, I still see LinalgTransformOps.cpp calling code from `Linalg/Transforms/Tiling.cpp`:
> 
> ```
>   FailureOr<linalg::ForallTilingResult> maybeTilingResult = failure();
>   if (!mixedNumThreads.empty()) {
>     maybeTilingResult =
>         linalg::tileToForallOp(rewriter, tileableOp, mixedNumThreads, mapping);
>   } else {
>     maybeTilingResult = linalg::tileToForallOpUsingTileSizes(
>         rewriter, tileableOp, mixedTileSizes, mapping);
>   }
> ```
> 
> Do you have a followup PR which cleans up the old?
> 
> Note, as part of this, I would also like to see something concrete on the `num_threads` discussion.

`num_threads` is the missing piece for me. I had asked this previously as to what is the difference between using loops of the form `[0, num_threads)` with the induction variable being some thread ID, vs. induction variable being `[lb, ub)` with `step`. It seems to be that the use of thread ID needs to come in only at the point of "resolution" of `scf.forall` but I maybe missing something. Again, treating it incrementally. Now with this PR I have the basics to start unwinding some of this.

> 
> Besides the meta-comment that we now need to resolve, I'll be able to review this early next week.
> 
> Thanks!

Thanks!

https://github.com/llvm/llvm-project/pull/77874