This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Derive padded subtensor shape from linalg op outputs
AbandonedPublic

Authored by springerm on May 25 2021, 6:28 PM.

Details

Reviewers
nicolasvasilache
Summary

Add a canonicalization pattern that rewrites memref::DimOps accordingly.

Depends On D103226

Diff Detail

Event Timeline

springerm created this revision.May 25 2021, 6:28 PM
springerm requested review of this revision.May 25 2021, 6:29 PM
nicolasvasilache requested changes to this revision.May 26 2021, 2:12 AM

Let's add a proper folding hook and use createAndFold please, this has multiple benefits over all clients using internal op-specific knowledge.

This revision now requires changes to proceed.May 26 2021, 2:12 AM

rebase + rewrite as canonicalization pattern

Let's add a proper folding hook and use createAndFold please, this has multiple benefits over all clients using internal op-specific knowledge.

The folder would have to be implemented as part of DimOp::fold in MemRefOps.cpp. It would have to check if the DimOp operand is a LinalgOp and if so, replace it with a new DimOp on the LinalgOp's respective output. This means putting linalg-specific code into the memref dialect.

A canonicalization pattern may be more suitable.

nicolasvasilache added a subscriber: silvas.

A folder would be better because createOrFold cleans a lot of things up by construction without relying on calling a canonicalization pass in weird places.
In this particular this is fine for now because coarse-grained canonicalization is fine.

Still, I wonder if this is the justification we've been wanting to get the TiedOperandInterface in place and refactor using this: @silvas @mehdi_amini for an extra set of eyes.

This revision is now accepted and ready to land.May 26 2021, 11:30 PM
springerm updated this revision to Diff 348208.May 27 2021, 3:32 AM

minor changes

Submitting this commit as is, but something like TiedOperandInterface would in fact be useful here. Will revisit this if we end up adding sth like TiedOperandInterface.

DimOp should already have a canonicalizer that will use InferShapedTypeOpInterface to rewrite dim(linalgop(x)) to some_expr_of(dim(x)). Do you have a test case where that doesn't happen?

One difference is that I think the current way that this canonicalization plays out is that it calculates the output shape in terms of the ins. Whereas what you are proposing here calculates it in terms of the outs (which is trivial). This came up in the past and @mravishankar argued for the "shape in terms of ins" approach, whereas I argued for the "shape in terms of outs" approach. I'll let you experts discuss this matter.

I'm generally against TiedOperandInterface (at least the way that IREE's is defined) because it has very vague semantics (it conflates a number of things -- the definition literally says something like "depending on the context it is interpreted as either A, B, or C" which doesn't have the precision needed to guide transformations safely). If the information needed is that the shape of an output is the same as the shape of an input, then InferShapedTypeOpInterface is already sufficient to express that.

You are right, the existing canonicalizations + CSE are good enough. This commit is not needed, but I didn't realize that until after I wrote the pattern ;)

springerm abandoned this revision.May 30 2021, 6:05 PM