This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Introduce folding patterns to remove certain MemRefCastOp
ClosedPublic

Authored by nicolasvasilache on Jan 21 2020, 3:50 PM.

Details

Summary
[mlir][Linalg] Introduce folding patterns to remove certain MemRefCastOp

Summary:
Canonicalization and folding patterns in StandardOps may interfere with the needs
of Linalg. This revision introduces specific foldings for dynamic memrefs that can
be proven to be static.

Very concretely:

Determines whether it is possible to fold it away in the parent Linalg op:

```mlir
  %1 = memref_cast %0 : memref<8x16xf32> to memref<?x?xf32>
  %2 = linalg.slice %1 ... : memref<?x?xf32> ...
  // or
  %1 = memref_cast %0 : memref<8x16xf32, affine_map<(i, j)->(16 * i + j)>>
         to memref<?x?xf32>
  linalg.generic(%1 ...) : memref<?x?xf32> ...
```

into

```mlir
  %2 = linalg.slice %0 ... : memref<8x16xf32> ...
  // or
  linalg.generic(%0 ... : memref<8x16xf32, affine_map<(i, j)->(16 * i + j)>>
```

Diff Detail

Event Timeline

Remove spurious pattern.
Fix comment.

Unit tests: pass. 62018 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

Harbormaster completed remote builds in B44536: Diff 239448.

Unit tests: pass. 62018 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

Fix the test, give up on a single pass in this iteration.

nicolasvasilache edited the summary of this revision. (Show Details)Jan 22 2020, 6:18 PM

Unit tests: pass. 62018 tests passed, 0 failed and 783 were skipped.

clang-tidy: pass.

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

This revision is now accepted and ready to land.Jan 23 2020, 7:47 AM
ftynse requested changes to this revision.Jan 23 2020, 8:00 AM
ftynse added inline comments.
mlir/test/Dialect/Linalg/matmul-to-vector.mlir
3 ↗(On Diff #239756)

Is this comment relevant to the test?

25 ↗(On Diff #239756)

Could you describe where do this magic sizes come from?

mlir/test/lib/DeclarativeTransforms/TestLinalgMatmulToVectorPatterns.td
3 ↗(On Diff #239756)

MLIR Project

mlir/test/lib/Transforms/TestLinalgMatmulToVector.cpp
27 ↗(On Diff #239756)

a MemrefCastOp, you don't change the LinalgOp

39 ↗(On Diff #239756)

I don't understand this comment.

If you have a memref_cast %0 fed by %0 = %0 = memref_cast %? : ... to memref<?x?xf32>, why do we convert it to memref_cast %0 : ... to memref<8x16xf32>`. There's something wrong with the order in your description.

I understand it even less after reading the code.

48 ↗(On Diff #239756)

Nit: cast

56 ↗(On Diff #239756)

Can verified memref_cast apply to non-memrefs?

81 ↗(On Diff #239756)

Leftover comment

83 ↗(On Diff #239756)

Nit: I'd put it before iterating over the use list

95 ↗(On Diff #239756)

Please add a comment, it looks like this block is useless (it isn't the trick is in the "linear" part).

112 ↗(On Diff #239756)

I understand this is a test code, but having a separate test that exercises this pattern specifically would definitely help. One one hand, avoid spurious failures in a bigger test. On the other hand, better see what kind of transformation you are doing here.

This revision now requires changes to proceed.Jan 23 2020, 8:00 AM
jsetoain added inline comments.Jan 23 2020, 10:37 AM
mlir/test/lib/Transforms/CMakeLists.txt
29 ↗(On Diff #239756)

I'm not sure if the policy is to keep these in lexycographical order?

mlir/test/lib/Transforms/TestLinalgMatmulToVector.cpp
3 ↗(On Diff #239756)

Part of the LLVM Project or part of the MLIR Project?

Thanks @ftynse I'll split out the new pattern in an independent test and rebase.

Note that comments in this diff are also relevant to the problem at hand: https://reviews.llvm.org/D73295.

tetuante added inline comments.Jan 24 2020, 2:26 AM
mlir/test/Dialect/Linalg/matmul-to-vector.mlir
25 ↗(On Diff #239756)

I think they come from

def : Pat<(MatmulOp:$op $_, $_, $_),
          (TileLinalgOp<[8, 12, 16], "L1__with_perm__", [1, 0, 2]>),
         [(Constraint<HasLinalgTransformMarker<"L2__with_perm__">>)]>;

in TestLinalgMatmulToVectorPatterns.td

They are fixed tile sizes right now for testing, but should somehow be computed in the futre

tetuante accepted this revision.Jan 24 2020, 2:26 AM
mehdi_amini marked an inline comment as done.Jan 25 2020, 8:01 PM
mehdi_amini added inline comments.
mlir/test/lib/Transforms/TestLinalgMatmulToVector.cpp
3 ↗(On Diff #239756)

Thanks for noticing. I just mass-updated all the MLIR files in the repo to mention "Part of the LLVM project".

nicolasvasilache retitled this revision from [mlir][Linalg] Add a Linalg DRR test to go from matmul to vectors to [mlir][Linalg] Introduce folding patterns to remove certain MemRefCastOp.Jan 28 2020, 10:49 AM
nicolasvasilache edited the summary of this revision. (Show Details)

Apologies for the delay, I recast this diff as an independent linalg canonicalization + test only as @ftynse suggested.
I'll be rebasing the rest on top of this in a new diff.

rriddle added inline comments.Jan 28 2020, 11:04 AM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
73

When will this ever be hit? The desiredResultType doesn't appear to have any maps.

78

nit: getUsers

102

nit: Can we use early exit here?

1173

Drop the llvm:: and mlir:: on all of these.

Unit tests: fail. 62196 tests passed, 1 failed and 815 were skipped.

failed: MLIR.mlir-cpu-runner/unranked_memref.mlir

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

nicolasvasilache marked 5 inline comments as done.Jan 28 2020, 12:17 PM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
78

incorrectly carried over, case removed.

nicolasvasilache marked an inline comment as done.
nicolasvasilache edited the summary of this revision. (Show Details)

Address review comments.

Unit tests: pass. 62197 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse accepted this revision.Jan 29 2020, 4:50 AM
ftynse added inline comments.
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
87–91

Let's exit even earlier

if (sourceType.getAffineMaps.empty())
  return true;

AffineMap sourceMap = sourceType.getAffineMaps().front();
100

return sourceMap == stridedMap ?

This revision is now accepted and ready to land.Jan 29 2020, 4:50 AM
jsetoain accepted this revision.Jan 29 2020, 5:32 AM

LGTM.

nicolasvasilache marked 2 inline comments as done.Jan 29 2020, 6:56 AM

Thanks for your reviews!

This landed here: https://github.com/llvm/llvm-project/commit/ea1e3369f7a8aa9729f8e2fc208b8f6a79392874

But since I made a mistake with phab I need to close manually.