This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Transform] Add ApplyPatternsOp
Changes PlannedPublic

Authored by springerm on Apr 26 2023, 3:42 PM.

Details

Summary

Adds transform dialect extension op ApplyPatternsOp from IREE#CommonExtensions to LinalgTransformDialectExtension.

All patterns come over smoothly except for cse (which could be brought over as well by adding ListenerCSE#CSE).

Only bike-sheddable part is where to put the helpers getWmmaNativeVectorSize, getMmaNativeVectorSize, and gpuMmaUnrollOrder; in IREE they live at compiler/Codegen/Utils/GPUUtils but MLIR doesn't have a proper "codegen" module/directory; Conversionis the closest semantical mapping IMHO and hence placing those helpers there. Note, given the name VectorToGPU/Utils, I could also transplant the multitude of helpers in VectorToGPU.cpp; they are all currently static "private" but some are probably generally useful.

Alternatively, especially in light of recent clamor for E2E codegen/runtime examples/functionality in MLIR itself, we could create a Codegen module/directory (maybe Conversion/Codegen). This would enable smoother upstreaming of many more useful things in IREE's compiler but maybe it's better those things are upstreamed in similar fashion (i.e., each thing in a related Conversion/XToY).

Diff Detail

Event Timeline

makslevental created this revision.Apr 26 2023, 3:42 PM
Herald added a project: Restricted Project. · View Herald Transcript

rename to ApplyPatternsExtension

makslevental retitled this revision from [MLIR][Transform] Add ApplyPatternsOp as first op in CommonExtensions to [MLIR][Transform] Add ApplyPatternsOp as Transform Extension.

amend commit message

move things around

makslevental retitled this revision from [MLIR][Transform] Add ApplyPatternsOp as Transform Extension to [MLIR][Transform] Add ApplyPatternsOp.

amend commit message

makslevental edited the summary of this revision. (Show Details)Apr 27 2023, 9:04 AM
makslevental published this revision for review.Apr 27 2023, 9:43 AM
ftynse requested changes to this revision.Apr 27 2023, 10:15 AM

This op doesn't belong to Linalg/structured op extension. Some of the changes also don't belong to the commit that introduces the op.

mlir/include/mlir/Conversion/VectorToGPU/Utils.h
1

This should be saying Utils.h and explaining what the file contains.

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.h
66

We don't commit commented-out code.

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

This really does not belong to LinagTransformOps. There is very little specific to Linalg, and it is the opposite of "structured transformation" approach that's being adopted by most of the ops here.

44–45

The op definition below doesn't return anything.

47–48

This is scary. In absence of benefit, the order of addition will affect the order in which patterns are checked and applied, which in turn affects the outcome.

49–50

Can this be more specific? "fancy patterns" doesn't really help anyone understand what this is supposed to be doing.

61–64

We don't commit commented-out code. Also, CSE is not patterns.

80–83

LICM is not patterns, doesn't look like it belongs here.

120–122

I don't think we want to expose the user to "listener" and other implementation details. This should be rewritten in terms of ops and tracking.

159

This shouldn't be necessary.

162

This TODO would be better next to the definition of ApplyPatternsOpPatterns.

mlir/lib/Conversion/VectorToGPU/Utils.cpp
1

This doesn't look related to the introduction of the transform op itself. Please split it out in a separate commit for review by relevant people and provide tests.

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

It would be significantly more helpful if this constructed an error message indicating for which op the replacement couldn't be found, stored in DiagnosedSilenceableFailure or captured in a callback.

218

Please document all top-level entities.

223

Does the vector need to be materialized here? Looks like one could keep a range.

231

This doesn't need to be a template parameter, a function argument would suffice.

247–287

These don't look like they belong here. They should probably go to something like lib/Dialect/Tensor/{Transforms | Utils}/Patterns.cpp.

296–331

Ditto. This can go into lib/Dialect/Linalg/Transform/???Patterns.cpp, there is already a precedent with other patterns in there.

334–345

Why are these even necessary? Can't the caller just directly call the inner function instead? There are many of similar cases below, none are documented.

361

We are upstream ;)

375

Commented-out code...

381

Ditto.

459

There's no strong reason for this to be a definite failure, we didn't mutate the IR, so we can have a silenceable faliure and move on.

545

We are upstream, consider confirming or revisiting this assumption.

552–553

Ditto.

This revision now requires changes to proceed.Apr 27 2023, 10:15 AM
springerm commandeered this revision.Jun 1 2023, 1:47 AM
springerm planned changes to this revision.
springerm edited reviewers, added: makslevental; removed: springerm.

I'm going to add an ApplyPatternsOp to the transform dialect, along with a mechanism to register patterns via transform dialect extensions.