[PATCH] D79765: [mlir][Linalg] Add folders and canonicalizers for linalg.reshape/linalg.tensor_reshape operations.

[Nicolas Vasilache via Phabricator via llvm-commits]

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

Looks good but please fix the test and address the TensorReshape / memref folding discrepancy.



================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:395
+      for (int i = 0, e = mapsProducer[dimExpr.getPosition()].getNumResults();
+           i != e; ++i) {
+        reassociations.push_back(getAffineDimExpr(currDim++, context));
----------------
`i < e` ?


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:415
+    auto srcReshapeOp =
+        dyn_cast_or_null<ReshapeOpTy>(reshapeOp.src().getDefiningOp());
+    if (!srcReshapeOp)
----------------
we now have `value.getDefiningOp<OpTy>()` that does this in a more idiomatic way (here and below)


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:1248
+OpFoldResult TensorReshapeOp::fold(ArrayRef<Attribute>) {
+  if (succeeded(foldMemRefCast(*this)))
+    return getResult();
----------------
I don't get this, `TensorReshape` operates on tensors ?


================
Comment at: mlir/test/Dialect/Linalg/llvm.mlir:227
+//       CHECK:    llvm.mlir.undef : !llvm<"{ float*, float*, i64, [5 x i64], [5 x i64] }">
+//       CHECK:    llvm.extractvalue %9[0] : !llvm<"{ float*, float*, i64, [3 x i64], [3 x i64] }">
+//       CHECK:    llvm.insertvalue %11, %10[0] : !llvm<"{ float*, float*, i64, [5 x i64], [5 x i64] }">
----------------
no hardcoded names (e.g. `%9`) in tests please: capture what you need and wildcard the rest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79765