Page MenuHomePhabricator

[mlir] Add MemRefReshapeOp definition to Standard.
ClosedPublic

Authored by pifon2a on Oct 20 2020, 2:46 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Oct 20 2020, 2:46 AM
pifon2a requested review of this revision.Oct 20 2020, 2:46 AM
herhut requested changes to this revision.Oct 21 2020, 5:29 AM
herhut added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2232

There is also a restriction on the number of elements, if known, right?

2235

nit: ranked or ranked?

2249

nit: here too.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2152

You have extraClassDeclaration so why not move this there, too?

2173

Is this tested somewhere?

This revision now requires changes to proceed.Oct 21 2020, 5:29 AM
ftynse added inline comments.Oct 21 2020, 9:04 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2235

Nit: s/length/size

2240–2241

I'm not sure why we need this version. The shape of the result is known from the type, so this is essentially asserting that the shape argument contains the same values as the result type.

2269

We don't do OpBuilder, OperationState here anymore.

2281–2282

Nit: would functional-type(operands, results) work here?

pifon2a updated this revision to Diff 299773.Oct 21 2020, 12:05 PM
pifon2a marked 9 inline comments as done.

Address the comments.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2232

Right. In the design doc @ftynse had the idea of having a separate pass that would add std.asserts to ensure the same number of elements.

2240–2241

I am not sure what you mean here. Do you mean that there is no need to print the shape if the result is statically-shaped and %shape is const? We need a case when the shape arg is non-constant. I'll add some examples.

2269

that's much nicer

herhut accepted this revision.Oct 22 2020, 4:25 AM
herhut added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2240–2241

I guess the idea would be to drop the shape operand if it is static anyway. That would give us a static and dynamic version of this operation. Could be worthwhile, especially as we otherwise have to allocate a memref with static contents.
One could also envision ranked/unranked versions that take scalars vs. memrefs.

I'd say we start with the most general form and can add refinements later if they turn out to be useful.

This revision is now accepted and ready to land.Oct 22 2020, 4:25 AM
This revision was automatically updated to reflect the committed changes.