[Mlir-commits] [mlir] [NFC] Simplify the tiling implementation using cloning. (PR #72178)

[llvmlistbot at llvm.org]

================
@@ -708,28 +781,46 @@ void mlir::scf::yieldReplacementForFusedProducer(
     RewriterBase &rewriter, tensor::ExtractSliceOp sliceOp,
     scf::SCFFuseProducerOfSliceResult fusedProducerInfo,
     MutableArrayRef<scf::ForOp> loops) {
-  auto [fusableProducer, fusedProducerValue, tileAndFusedOps] =
-      fusedProducerInfo;
-  SmallVector<Value> initValues;
+  if (loops.empty()) {
----------------
MaheshRavishankar wrote:

> None of this code is actually used in MLIR besides a test.
> 
> Is there a plan to use this in some other implementation (e.g. fusion in LinalgTransformOps) or are we in another occurrence of multiple implementations?
> 
> In fact, I see in `TestTilingInterface::TestTileConsumerFuseAndYieldProducerUsingSCFForOp` that:
> 
> ```
>     // The rest of this method is similar to
>     // scf::tileConsumerAndFuseProducerGreedilyUsingSCFForOp, except that also
>     // yields replacements for values of the fused producer.
> ```
> 
> Why do we need this / should we replace `tileConsumerAndFuseProducerGreedilyUsingSCFForOp` by this (it seems to have more use cases) ?
> 
> Skipping the review of this commit until we get clarification on the intended direction.

Yes, I agree this part of the code is awkward and this is not what I would do either. I think this was just a working compromise. I wanted to fix that up as well, ill describe below how, but my aim with this PR is too remove the `yieldTiledValues` and `updateDestinationOperandsForTiledOp` which never really worked well, and I figured how to fix that only after seeing it work better using the cloning approach. So for now, I'd rather land this PR as an NFC (which it is, is there something that stands out to you as not being NFC, cause that is not the intent)

Coming back to this method, I would rather remove the somewhat artificially split up that introduces `tileAndFuseProducerOfSlice`  and `yieldReplacementForFusedProducer`. (FWIW I guess the split isnt too bad cause I changed the implementation underneath without having to change these methods, which is something). But I want to support the use case that is represented by the test here https://github.com/llvm/llvm-project/blob/main/mlir/test/Interfaces/TilingInterface/tile-fuse-and-yield-using-interface.mlir . Without going into details, the intent here is that when you tile and fuse operations, in some cases you also want to return a replacement for the fused producer (just like the tiling returns a replacement for the original untiled operation). Reason it is important is consider a function that is returning both the consumer op and the producer op. If you are tiling the consumer and fusing the producer with it, if you dont return a replacement value for the producer, the original producer stays live (and that actually defeats the purpose of the fusion).

So my proposed fix up (as a follow up), is to allow an extra list of values passed as arguments to `tileConsumerAndFuseProducerGreedily..` . If this value is tiled and fused , the loop will return a replacement value for it. I can send out a draft PR for it on top of this if this helps, but I hope we can decouple the two. This PR should be purely NFC.



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