This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Add a linalg.reshape op
ClosedPublic

Authored by nicolasvasilache on Jan 3 2020, 9:22 AM.

Details

Summary

This diff adds a new operation to linalg to allow reshaping of an
existing view into a new view in the same buffer at the same offset.

More specifically:
The linalg.reshape op produces a new view whose sizes are a reassociation
of the original view. Depending on whether or not the reassociated
MemRefType is contiguous, the resulting memref may require explicit alloc
and copies.

A reassociation is defined as a continous grouping of dimensions and is
represented with a affine map array attribute. In the future, non-continous
groupings may be allowed (i.e. permutations, reindexings etc).

For now, it is assumed that either:

  1. a reassociation produces and consumes contiguous MemRefType or,
  2. the reshape op will be folded into its consumers (by changing the shape of the computations).

All other cases are undefined behavior and a reshape op may not lower to
LLVM if it cannot be proven statically that it does not require alloc+copy.

A reshape may either collapse or expand dimensions, depending on the
relationship between source and target memref ranks. The verification rule
is that the reassociation maps are applied to the memref with the larger
rank to obtain the memref with the smaller rank. In the case of a dimension
expansion, the reassociation maps can be interpreted as inverse maps.

Examples:

mlir
   // Dimension collapse (i, j) -> i' and k -> k'
   %1 = linalg.reshape %0 [(i, j, k) -> (i, j), 
                           (i, j, k) -> (k)] :
     memref<?x?x?xf32, stride_spec> into memref<?x?xf32, stride_spec_2>
mlir
   // Dimension expansion i -> (i', j') and (k) -> (k')
   %1 = linalg.reshape %0 [(i, j, k) -> (i, j), 
                           (i, j, k) -> (k)] :
     memref<?x?xf32, stride_spec> into memref<?x?x?xf32, stride_spec_2>

The relevant invalid and roundtripping tests are added.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2020, 9:22 AM

Unit tests: pass. 61176 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rriddle requested changes to this revision.Jan 3 2020, 10:04 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgOps.td
109

Do not add more uses of -> or *, this is just giving me more work.

mlir/include/mlir/IR/AffineExpr.h
248

return (!*this || !isa<U>()) ? U(nullptr) : U(expr);

mlir/include/mlir/IR/Attributes.h
1444 ↗(On Diff #236081)

Seems like it would be better to have something more general on ArrayAttr that can unwrap internal attributes. Preferably by returning a range instead of creating a vector.

mlir/include/mlir/IR/Builders.h
381 ↗(On Diff #236081)

I don't see a need for this.

mlir/lib/IR/StandardTypes.cpp
15

These paths make no sense.

589

This needs a comment, specifically I don't see why we need to do all of this simplification here.

594

Why by-reference?

746

These names seem way too general for what they are used for. Why are these free functions and not methods of MemRefType?

This revision now requires changes to proceed.Jan 3 2020, 10:04 AM
kiszk added a subscriber: kiszk.Jan 3 2020, 10:06 AM
kiszk added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgOps.td
72

nit: a -> an

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

maybe enough -> have enough?

nicolasvasilache marked 6 inline comments as done.

Rebase and address comments.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

nicolasvasilache marked 8 inline comments as done.

Address review comments.

mlir/include/mlir/IR/Attributes.h
1444 ↗(On Diff #236081)

Sorry I have to punt on this one, I looked at the iterator_facade stuff but this exceeds my immediate C++ skills.

mlir/lib/IR/StandardTypes.cpp
589

This was a bad rebase, should be good now.

594

This was a bad rebase, should be good now.

746

It's unnecessary, killing.

Update commit message.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

rriddle added inline comments.Jan 3 2020, 2:51 PM
mlir/include/mlir/IR/StandardTypes.h
12

This should be avoided as much as possible. We should avoid adding more includes to headers, especially common ones.

590

This looks unused.

mlir/lib/IR/StandardTypes.cpp
715

Why do these have to be re-simplified? Seems like this could just be done on construction, what is the benefit of doing this here?

nicolasvasilache marked 5 inline comments as done.Jan 3 2020, 3:10 PM
nicolasvasilache added inline comments.
mlir/include/mlir/IR/StandardTypes.h
12

ugh .. of course, thanks!

mlir/lib/IR/StandardTypes.cpp
715

I can avoid this and just return the original type for now.
I do not remember the rationale for why simplifyAffineMap cannot be run by default but I remember opposition 1+ year ago.
It may be related to roundtripping and old ways of writing tests.

nicolasvasilache edited the summary of this revision. (Show Details)Jan 3 2020, 3:11 PM
nicolasvasilache marked 2 inline comments as done.
nicolasvasilache edited the summary of this revision. (Show Details)

Address review comments.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Thanks! Anything else?

Unit tests: pass. 61250 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Landed the current version. I will address extra requests, if any, in a followup.
I have a separate diff with proper iterators and ranges, I'll update the AffineMap extraction part once https://reviews.llvm.org/D72310 lands.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 6 2020, 7:28 PM
This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 61250 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml