[PATCH] D148159: [mlir][TransformDialect] Simplify the lowering of pack/unpack when these are just pad/unpad

Thu Apr 13 02:37:50 PDT 2023

qcolombet marked 10 inline comments as done.
qcolombet added inline comments.

================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:839
+        /*strides=*/ones);
+    LLVM_DEBUG(DBGSNL(); DBGSNL(); DBGSNL();
+               DBGS() << "insert_slice op: " << replacementOp; DBGSNL(););
----------------
nicolasvasilache wrote:
> 3 newlines ? :)
It was like that in the exiting code :).
Removed it.

================
Comment at: mlir/test/Dialect/Linalg/transform-lower-pack.mlir:41
+  //      CHECK: %[[EMPTY:.*]] = tensor.empty() : tensor<1x1x1x1x136x64x16x16xf32>
+  //      CHECK: %[[RES:.*]] = tensor.insert_slice %[[PAD]] into %[[EMPTY]]
+  // offsets.
----------------
nicolasvasilache wrote:
> mravishankar wrote:
> > I wondering if we even want to have an `insert_slice` here? we could just do a `reshape` here. In which case the only difference is whether there is a transpose or not? If we can teach bufferization to handle this reshape without introducing additional buffers, that'd be very useful (we used to have a `memref.reshape` to just change the indexing, wonder if we still have that).
> In principle we could but I'd like to see how these things connect in practice.
> 
> At the graph level, reshapes tend to simplify reasonably well.
> At the codegen level, as soon as we start tiling and taking slices, reshapes don't behave properly atm.
> 
> I'd say this is dependent on where we apply the lowering and we will likely want this to be configurable in the future, based on data.
> 
Alright I'll land as is and we'll revisit later then.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148159/new/

https://reviews.llvm.org/D148159