[Mlir-commits] [mlir] [MLIR] Don't drop attached discardable attributes (PR #111261)

[Han-Chung Wang]

================
@@ -4337,11 +4337,16 @@ LogicalResult PackOp::canonicalize(PackOp packOp, PatternRewriter &rewriter) {
       dest =
           rewriter.create<tensor::CastOp>(loc, newDestType, packOp.getDest());
     }
-    Value newOp = rewriter.create<tensor::PackOp>(
-        loc, source, dest, packOp.getInnerDimsPos(), packOp.getMixedTiles(),
-        packOp.getPaddingValue(), packOp.getOuterDimsPerm());
+    auto clonedPackOp = cast<PackOp>(rewriter.clone(*packOp));
----------------
hanhanW wrote:

I think it is not trivial to do in-place modification. There are two situations:

1. The dest tensor type is the same. In this case, we do not need a tensor.cast consumer.
2. The dest tensor type is changed. In this case, we need to create the tensor.cast op which makes the types consistent.

In the (1) situation, we can do in-place modification -- which is very simple.

In the (2) situation, it is not trivial because you need to replace the original op with the new tensor.cast op. If we do in-place modification, I don't see a trivial way to replace the op. Perhaps we can replace the uses of the tensor.pack ops with the new tensor.cast op, when it is the case.

IMO, cloning an op is cheap in this case. Instead of adding complex to logics, I'm +1 on cloning the op approach. 

Note that this is also what we're doing for LinalgOps and it's been there for a couple years. I'm not saying that this is the correct way, but it's more like providing data points.

https://github.com/llvm/llvm-project/blob/9f3c55954eaa71910caa0abbb404db02d3a104c1/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp#L2561-L2575


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