Page MenuHomePhabricator

[mlir][linalg] Introduce a separate EraseIdentityCopyOp Pattern.
ClosedPublic

Authored by gysit on Jul 8 2021, 4:30 AM.

Details

Summary

Split out an EraseIdentityCopyOp from the existing RemoveIdentityLinalgOps pattern. Introduce an additional check to ensure the pattern checks the permutation maps match. This is a preparation step to specialize RemoveIdentityLinalgOps to GenericOp only.

Diff Detail

Event Timeline

gysit created this revision.Jul 8 2021, 4:30 AM
gysit requested review of this revision.Jul 8 2021, 4:30 AM
nicolasvasilache accepted this revision.Jul 28 2021, 2:32 AM
This revision is now accepted and ready to land.Jul 28 2021, 2:32 AM
gysit updated this revision to Diff 362311.Jul 28 2021, 2:41 AM

Rebase.

rriddle added inline comments.Jul 28 2021, 10:47 AM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
451

An assert like this in a canonicalization pattern feels pretty sketchy. Canonicalization patterns can always be applied to an op, so when is this not true?

gysit added inline comments.Jul 28 2021, 11:35 AM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
451

The input and output operands are limited to memrefs in the op definition and the assertion is always true.

It is here since the canonicalization only makes sense in buffer land and it would be triggered if the op is modified to work on tensors (as most other structured operations). It thus encodes a necessary precondition to make the canonicalization correct.

mehdi_amini added inline comments.Jul 28 2021, 11:51 AM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
451

If I understand correctly, you're saying that this assert could be inserted in the verifier for CopyOp as well?

gysit added inline comments.Jul 28 2021, 12:21 PM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
451

The verification already checks the input and output operands are memrefs (they are of type AnyStridedMemRef). I think we are good there.

The purpose of the assert is to mark that this particular piece of code breaks once the operation supports tensor semantics. I personally can live with removing it but I would say it is a safety net and it also helps to understand the code. Comparing an output tensor otherwise may seem strange otherwise...

mehdi_amini added inline comments.Jul 28 2021, 8:07 PM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
451

My question was to validate my understanding: "in the current state of this op, this check could be added to the verifier and nothing would break"?

You're saying "The verification already checks the input and output operands are memrefs", but it isn't obvious to a reader (like River here) that this implies the same thing as this assert, hence my clarifying question.

gysit added inline comments.Jul 28 2021, 11:14 PM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
451

Ah, yes the hasBufferSemantics method checks that all operands are memrefs or scalars.

Sorry too deep in the topic :).