This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add TransposeOp to Linalg structured ops.
ClosedPublic

Authored by olegshyshkov on Oct 13 2022, 12:47 AM.

Diff Detail

Event Timeline

olegshyshkov created this revision.Oct 13 2022, 12:47 AM
olegshyshkov requested review of this revision.Oct 13 2022, 12:47 AM

clang-format

pifon2a accepted this revision.Oct 13 2022, 2:40 AM

Nice!

mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
1851

super-nit: ++indexCounts[index]?

This revision is now accepted and ready to land.Oct 13 2022, 2:40 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
363

@jpienaar, is ReifyRankedShapedTypeOpInterface enough for the type inference we need for Linalg Ops ?

364

Can you also add DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>, here and make the result SSA name be called "transposed" ?

This will improve readability of IR.

369

Please add doc that unlike memref.transpose which is a metadata operation only that produces a transposed "view", this op actually moves data.

376

nit: I think I would put the permutation attr before the ins

384

why not call output ?

393

I would make both builders not take a resultType and use the shape reification mechanism.
There is no possible type polymorphism in this op.

403

why do we also need this?

404

why not just return getInput() ?

405

why not just getInit ?

Address some comments.

olegshyshkov marked 8 inline comments as done.Oct 14 2022, 5:32 AM
olegshyshkov added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
376

I'm not sure about that. I would prefer to have ins and out before attributes for clarity and consistency with ReduceOp, but it's just my mental model.

olegshyshkov added inline comments.Oct 14 2022, 5:36 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
384

I know that there is a bigger discussion if it should init or output in DPS, but here I used init to be consistent with Reduce and Map.

I can rename init into output in other ops in a separate patch if you prefer.

olegshyshkov marked 2 inline comments as done.Oct 18 2022, 5:30 AM
olegshyshkov added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
363

Looks like this comment is not strictly related to this change, so I'll mark it as Done here.

393

Thanks to DPS I was able to simple add odsState.addTypes(init.getType()); to the builder.

This revision was automatically updated to reflect the committed changes.
olegshyshkov marked an inline comment as done.