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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added a comment.
This revision is now accepted and ready to land.

can we add a test with dynamic shape ?
(maybe we can't if some op does not support it, but I *think* we can)



================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:833
+    for (int64_t dstSize : origShape)
+      sizes.push_back(rewriter.getIndexAttr(dstSize));
+
----------------
This assumes static, we really want `createOrFold<DimOp>` here; we should have some helpers around that return the vector<OpFoldResult> with the right quantities.
Not sure where to look offhand for those helpers ..


================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:839
+        /*strides=*/ones);
+    LLVM_DEBUG(DBGSNL(); DBGSNL(); DBGSNL();
+               DBGS() << "insert_slice op: " << replacementOp; DBGSNL(););
----------------
3 newlines ? :)


================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:840
+    LLVM_DEBUG(DBGSNL(); DBGSNL(); DBGSNL();
+               DBGS() << "insert_slice op: " << replacementOp; DBGSNL(););
+  } else {
----------------
can we early exit here and avoid the else nesting ?


================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:864
+  }
+  assert(padOp && replacementOp &&
+         (packOp.isLikePad() || (reshapeOp && transposeOp)) &&
----------------
with early exit, I believe we can dropp the trailing assert.


================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:933
+    for (int64_t dstSize : destShape) {
+      sizes.push_back(rewriter.getIndexAttr(dstSize));
+    }
----------------
same re dynamic sizes


================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:998
+
+  assert(extractSliceOp &&
+         (unPackOp.isLikeUnPad() || (emptyOp && transposeOp && reshapeOp)) &&
----------------
same re early exit and dropping the trailing assert.


================
Comment at: mlir/lib/Dialect/Tensor/IR/TensorOps.cpp:3722
+  int64_t expectedDimIdx = 0;
+  ArrayRef<int64_t> innerDimsPos = packOp.getInnerDimsPos();
+  for (int64_t dimIdx : innerDimsPos) {
----------------
Something more idiomatic with `ArrayRef == llvm::seq` ?


================
Comment at: mlir/lib/Dialect/Tensor/IR/TensorOps.cpp:3741
+  // ones are ones.
+  for (int i = 0; i != packedRank - numPackedDims; ++i) {
+    if (packedShape[i] != 1)
----------------
Something like `return llvm::all_of(..., [](){ == 1; })` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148159