This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg][transform] Add TileOp to transform dialect
ClosedPublic

Authored by springerm on Apr 29 2022, 2:15 AM.

Details

Summary

This commit adds a tiling op to the transform dialect as an external op.

Diff Detail

Event Timeline

springerm created this revision.Apr 29 2022, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 2:15 AM
springerm requested review of this revision.Apr 29 2022, 2:15 AM
springerm updated this revision to Diff 425999.Apr 29 2022, 2:28 AM

Fix CMake build

ftynse added inline comments.Apr 29 2022, 3:57 AM
mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.h
2

Nit: wrong filename

14

Nit: wouldn't OpDefinition.h suffice in the header?

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
10

These aren't really linear algebra operations, but transforms thereof.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
64

function_ref (std::function is only useful if the callee needs to outlive the caller)

67–68

Is it possible to reserve space for these?

71

I'd rather dyn_cast and report a user-visible error here.

82–84

Nit: drop trivial braces.

88–90

Nit: drop trivial braces.

118

Ultra-nit: we tend to use single-quotes rather than backticks in user-visible text.

121

Don't we have something like getSizesAttrName() to avoid magic strings?

124

Nit: there is no need to use formatv to insert one statically known string into another statically known string, just write the full string.

I suppose this could have been taken from some other place that used getSizesAttrName() instead of hardcoding the attribute name. Even in that case, something like parser.emitError(opLoc) << "expected " << getSizesAttrName() << " attribute"; is probably cheaper / easier for the compiler to optimize than formatv.

160

We also need Read/Write effect on PayloadIRResource, not attached to a particular value.

springerm updated this revision to Diff 426024.Apr 29 2022, 5:19 AM
springerm marked 12 inline comments as done.

address comments

springerm added inline comments.Apr 29 2022, 5:21 AM
mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.h
14

Actually requires OpImplementation.h because OpAsmOpInterface is needed.

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
10

Ah, this was copy pasta...

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
121

We do, but getSizesAttrName is a member function and TileOp::parse() is static.

ftynse accepted this revision.Apr 29 2022, 5:30 AM
ftynse added inline comments.
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
121

Isn't there a static version that takes OperationName as argument? You can get OperationName from OperationState.

This revision is now accepted and ready to land.Apr 29 2022, 5:30 AM
springerm updated this revision to Diff 426025.Apr 29 2022, 5:35 AM
springerm marked an inline comment as done.

address comments

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
121

Nice I didn't know that

This revision was landed with ongoing or failed builds.Apr 29 2022, 5:39 AM
This revision was automatically updated to reflect the committed changes.