This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Create memref dialect and move several dialect-specific ops from std.
ClosedPublic

Authored by dfki-jugr on Feb 10 2021, 8:28 AM.

Details

Summary

Create the memref dialect and move several dialect-specific ops without
dependencies to other ops from std dialect to this dialect.
Note: The ops haven't been removed from std dialect yet.

The roadmap to split the memref dialect from std is discussed here:
https://llvm.discourse.group/t/rfc-split-the-memref-dialect-from-std/2667

Diff Detail

Event Timeline

dfki-jugr created this revision.Feb 10 2021, 8:28 AM
dfki-jugr requested review of this revision.Feb 10 2021, 8:28 AM
mehdi_amini added inline comments.Feb 10 2021, 1:07 PM
mlir/include/mlir/Dialect/MemRef/IR/MemRef.h
77

Could/should it be a method on the CastOp instead?

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
881

This introduces duplicated code in the repository, most of this is actually "moved" conceptually. Could we actually move the code and avoid duplication?

rriddle added inline comments.Feb 10 2021, 1:09 PM
mlir/include/mlir/Dialect/MemRef/IR/MemRef.h
77

+1

mlir/include/mlir/Dialect/MemRef/IR/MemRefBase.td
22

Drop the newline before and after this paragraph.

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
881

+1

silvas added inline comments.Feb 10 2021, 3:19 PM
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
881

+1

silvas added inline comments.Feb 10 2021, 3:20 PM
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
881

Personally I found that the cleanups involved in moving the ops were actually most of the work. Just copying the ops first gives a false impression that the work has been done.

dfki-jugr added inline comments.Feb 11 2021, 1:06 AM
mlir/include/mlir/Dialect/MemRef/IR/MemRef.h
77

The method uses a CastOp?
Or do you mean a different one?

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
881

The idea was to split the moving process into two steps: first adding the ops and second remove them from std. This should reduce review overhead, since it leads to a huge CL, otherwise. But sure, to avoid code duplication, it makes sense to have it in one step.

ftynse added a subscriber: ftynse.Feb 11 2021, 1:10 AM
ftynse added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
881

Phabricator highlights the code that was moved, and so do most git clients, so it is actually easier for reviewers and even the authors to do the change in one commit. For example, I would likely ignore the code that was just moved between files in the review, but would complain about things that appear as new code, asking the author to change them even if they didn't write them.

mehdi_amini added inline comments.Feb 11 2021, 11:27 AM
mlir/include/mlir/Dialect/MemRef/IR/MemRef.h
77

I mean that instead of a free function: bool canFoldIntoConsumerOp(CastOp castOp);

It seems like this could better be a predicate as a method on the class itself: bool CastOp::canFoldIntoConsumerOp() { ... }

Removed corresponding ops in std dialect and adapted test cases.
Addressed comments.

This revision is now accepted and ready to land.Feb 12 2021, 11:29 AM
silvas added inline comments.Feb 12 2021, 5:56 PM
mlir/test/IR/core-ops.mlir
626 ↗(On Diff #323236)

please move all uses of the memref dialect in mlir/test/IR to their own proper locations inside test/Dialect/MemRef. See what I did for the tensor dialect for examples.

silvas requested changes to this revision.Feb 12 2021, 5:56 PM
This revision now requires changes to proceed.Feb 12 2021, 5:56 PM
silvas accepted this revision.Feb 12 2021, 6:03 PM
silvas added inline comments.
mlir/test/IR/core-ops.mlir
626 ↗(On Diff #323236)

Actually, looks like we have accumulated some technical debt here. Looks like some of these tests need to be moved too. E.g. test/IR/slice.mlir looks like it needs to move to linalg or something, and a lot of the code in parser.mlir needs to be moved to somewhere in the affine dialect tests.

But it looks like memory-ops.mlir, invalid-ops.mlir, and core-ops.mlir changes make sense to move into test/Dialect/MemRef

(given this complexity, seems like a separate patch might be better; please make sure to do that follow-up though)

This revision is now accepted and ready to land.Feb 12 2021, 6:03 PM
herhut accepted this revision.Feb 15 2021, 2:04 AM

+1 to landing and then cleaning up tests.

This revision was landed with ongoing or failed builds.Feb 18 2021, 2:32 AM
This revision was automatically updated to reflect the committed changes.