This is an archive of the discontinued LLVM Phabricator instance.

[mlir][transform] Add TransformRewriter
ClosedPublic

Authored by springerm on Jun 8 2023, 1:29 AM.

Details

Summary

All apply functions now have a TransformRewriter & parameter. This rewriter should be used to modify the IR. It has a TrackingListener attached and updates the internal handle-payload mappings based on rewrites.

Implementations no longer need to create their own TrackingListener and IRRewriter. Error checking is integrated into applyTransform.

Additional API will be added to TransformRewriter in subsequent revisions. This revision just adds an "empty" TransformRewriter class and updates all apply implementations.

Depends On: D152446

Diff Detail

Event Timeline

springerm created this revision.Jun 8 2023, 1:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
springerm requested review of this revision.Jun 8 2023, 1:29 AM
ftynse added a comment.Jun 9 2023, 1:12 AM

Documentation needs to be updated, in particular the tutorial and potentially the top-level Dialects/Transform.md description to account for this.

I am not entirely sure what contract do we want to have between the rewriter and the apply implementation. While it is tempting to have a blanket rule of the style "all IR modifications in apply must go through the rewriter, everything else is UB", most existing transformations don't follow that. Also, the replacement tracking is limited and we may create cases where replacement failed, but it wasn't actually necessary and the transform would have succeeded without it, i.e., without using the rewriter. What are your thoughts on this?

mlir/examples/transform/Ch2/lib/MyExtension.cpp
86–87

Please update the actual tutorial as well, it lives in mlir/docs/Tutorials/transform/. It should remain in sync with these sources.

mlir/include/mlir/Dialect/Transform/IR/MatchInterfaces.h
53–54

Nit: namespace prefixing is not necessary here.

94–95

Ditto.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
973

Please add doxygen documentation for this.

974

Why does this need to be an extension? So it can access TransformState::replacePayloadOp? If that's the only reason, let's just make it a friend of TransformState or even make replacePayloadOp public with a disclaimer that it's a low-level API and that reasonable clients should use the rewriter instead. Extensions were meant to carry extra state, but also were separating the ugliness of the early attempts at tracking. Since tracking becomes more first-class, I don't see the reason of jumping through hoops with this.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.td
52

Update the documentation above to indicate that the implementation is expected to use the provided rewriter. It is also worth mentioning that its insertion point is not set and the implementation is in charge of setting it correctly given the payloads.

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
920

At this point, result may be a definite failure and we cannot attach notes to those (the diagnostic has been already reported). I suppose we can report a second error in this case.

1197

Nit: braces are required if there is a nested control flow construct.

1312

Nit: there is no op in the function below.

1316

Nit: I'd rather call this UB than invalid IR. Invalid IR suggests that it can be checked by the verifier but it cannot.

springerm updated this revision to Diff 530862.Jun 13 2023, 4:31 AM
springerm marked 9 inline comments as done.

addres comments

I am not entirely sure what contract do we want to have between the rewriter and the apply implementation. While it is tempting to have a blanket rule of the style "all IR modifications in apply must go through the rewriter, everything else is UB",

I was aiming for "everything has to go through the rewriter". Otherwise, it will be difficult to understand when a rewriter must be used and when it can be done without.

We have a similar situation with RewritePatterns. Creating a new with the rewriter, but then modifying it in-place (without updateRootInPlace) is actually fine (for the GreedyPatternRewriteDriver at least), but I always discouraged that when talking to people, because it makes the API more complicated and they may easily miss a required updateRootInPlace.

most existing transformations don't follow that.

Yes, I would update these gradually. (I think we have the same issue with some SCF RewritePatterns that call some util functions that just create a new OpBuilder...)

Also, the replacement tracking is limited and we may create cases where replacement failed, but it wasn't actually necessary and the transform would have succeeded without it, i.e., without using the rewriter. What are your thoughts on this?

I have a WIP change that allows users to install a custom replacement op finder:

rewriter.withPayloadReplacementHandler([&]() {
  //rewrite
}, /*handler=*/ [](ValueRange values) -> FailureOr<Operation *> {
 ...
});

There's also a default handler that silences all replacement errors:

rewriter.silencePayloadReplacementErrors([&]() { ... });
mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
974

Ah yes this does not have to be an extension. The listener is an extension already.

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
1312

This refers to op of notifyOperationReplaced.

Okay, my overall concern here is we may be breaking things that currently work in subtle ways. Both declaring the direct IR mutation as UB and by doing more tracking than before potentially aborting the transformation. If we can mitigate these two by (1) giving some slack for transformations/downstreams to adapt to using the rewriter + posting a PSA and (2) silencing replacement failures by default, without any action from the user, and separately discussing a switch in that behavior with another PSA, this should be fine.

ftynse accepted this revision.Jun 19 2023, 4:19 AM
This revision is now accepted and ready to land.Jun 19 2023, 4:19 AM
This revision was automatically updated to reflect the committed changes.