This is an archive of the discontinued LLVM Phabricator instance.

Promote transpose from linalg to standard dialect
ClosedPublic

Authored by bkramer on Oct 1 2020, 6:39 AM.

Details

Summary

While affine maps are part of the builtin memref type, there is very
limited support for manipulating them in the standard dialect. Add
transpose to the set of ops to complement the existing view/subview ops.
This is a metadata transformation that encodes the transpose into the
strides of a memref.

I'm planning to use this when lowering operations on strided memrefs,
using the transpose to remove the stride without adding a dependency on
linalg dialect.

Diff Detail

Event Timeline

bkramer created this revision.Oct 1 2020, 6:39 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
bkramer requested review of this revision.Oct 1 2020, 6:39 AM
This revision is now accepted and ready to land.Oct 1 2020, 7:00 AM
bondhugula requested changes to this revision.Oct 1 2020, 8:03 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
3163

Should view be named in here? It's not clear why the input should be called view.

3168

original view or the input memref?

3174

Since this is an example, consider having valid MLIR here - i.e., replacing stride_spec with an example spec itself for this memref.

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

This is missing a doc comment.

3395

parse and print methods are missing customary examples for syntax reference.

This revision now requires changes to proceed.Oct 1 2020, 8:03 AM
bondhugula added inline comments.Oct 1 2020, 8:05 AM
mlir/test/Dialect/Standard/invalid.mlir
95

Again, view isn't clear to me here.

+1 on moving this to std

this will simplify lowering paths that don't really need linalg (or already lowered that) quite a bit

mlir/docs/Dialects/Linalg.md
559

shall we move this up so that now all std are grouped together?

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
3032

is this comment place well? it seems we are just setting up some local vars here

mehdi_amini added inline comments.Oct 1 2020, 10:39 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
3393

What is missing in the declarative assembly format for this op?

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

+1 this op predates the declarative assembly format and if we can kill code the better.

bkramer updated this revision to Diff 295790.Oct 2 2020, 4:44 AM
bkramer marked 8 inline comments as done.
  • Address a round of review comments
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
3163

Renamed it to in.

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
3032

It's obsolete.

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

assemblyFormat handles affine maps differently, so the result would look like this:

transpose %0 affine_map<(i, j) -> (j, i)>

or this:

transpose %0 #map0

I don't know how important the actual syntax is or if we should add support to assemblyFormat to also handle 'inline' affine maps?

3395

Added a assemblyFormat-like string.

bondhugula resigned from this revision.Oct 3 2020, 1:33 PM
bondhugula added inline comments.
mlir/test/Dialect/Linalg/standard.mlir
58–60

Should the std prefix be dropped here? Ops below (memref_cast's) don't have it. Are these tests passing?

This revision is now accepted and ready to land.Oct 3 2020, 1:33 PM
bkramer updated this revision to Diff 296084.Oct 4 2020, 4:02 PM
bkramer marked an inline comment as done.
  • Don't print "std." prefix on tranpose
mlir/test/Dialect/Linalg/standard.mlir
58–60

Good catch. The other std ops drop it in their print methods.

This revision was automatically updated to reflect the committed changes.