This is an archive of the discontinued LLVM Phabricator instance.

[mlir][interfaces] Add DimLikeOpInterface
ClosedPublic

Authored by springerm on Sep 9 2022, 11:24 AM.

Details

Summary

This interface is implemented by memref.dim and tensor.dim. This change makes it possible to remove a build dependency of the Affine dialect on the Tensor dialect (and maybe also the MemRef dialect in the future).

Diff Detail

Event Timeline

springerm created this revision.Sep 9 2022, 11:24 AM
springerm requested review of this revision.Sep 9 2022, 11:24 AM
bondhugula added inline comments.Sep 10 2022, 5:38 PM
mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
489–492

Missing doc comments.

mlir/include/mlir/Interfaces/ShapedOpInterfaces.td
25–26

Copy/paste error. We'll need proper documentation for this interface.

ftynse added inline comments.Sep 13 2022, 5:35 AM
mlir/include/mlir/Interfaces/ShapedOpInterfaces.h
1

Nit: this line needs to have -*- C++ -*- somewhere to allow some editors to differentiate C and C++ headers.

21

Nit: this needs a doc comment.

mlir/include/mlir/Interfaces/ShapedOpInterfaces.td
23

Bikeshed: how about ShapedDimOpInterface? Makes it clear where the op belongs and the "something-like" implies the reader has previous knowledge of "something".

mlir/lib/Interfaces/ShapedOpInterfaces.cpp
18–22

Are these tested somewhere?

zero9178 added inline comments.
mlir/include/mlir/Interfaces/ShapedOpInterfaces.td
36

These should be fully qualified, as otherwise any downstream users overwriting these methods will get a compiler error unless they're using using namespace mlir;

44
nicolasvasilache accepted this revision.Sep 23 2022, 4:14 AM

Thanks much @springerm , LGTM once other comments are addressed.

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
11

What is still required to sever this dependence?

This revision is now accepted and ready to land.Sep 23 2022, 4:14 AM
springerm updated this revision to Diff 464599.Oct 2 2022, 9:53 PM
springerm marked 7 inline comments as done.

address comments

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
11

There are references to memref::CastOp, memref::GetGlobalOp, memref::ViewOp, memref::SubViewOp, memref::AllocOp in AffineOps.cpp.

Functions that must be updated: AffineLoadOp::fold, AffineStoreOp::fold and isDimOpValidSymbol. Not sure how easy/difficult this would be. The folders are basically cross-dialect canonicalizations.

mlir/lib/Interfaces/ShapedOpInterfaces.cpp
18–22

It is not tested at the moment. Requires a fair bit of boilerplate code to set up dummy ops etc., so not worth it IMO. I can add a test case in a follow-up change if you'd prefer to have one.

This revision was landed with ongoing or failed builds.Oct 2 2022, 9:59 PM
This revision was automatically updated to reflect the committed changes.