[mlir][linalg] Do not set insertion point inside padding function #165420
Conversation
Groverkss
requested review from
dcaballe,
nicolasvasilache and
rengolin
as code owners
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir Author: Kunwar Grover (Groverkss) ChangesRemove insertion point in rewriteAsPaddedOp. There is no gurantee that the sizes provided by the user are before the operation to pad. It's better to let the user handle where to insert the newly created operations, as long as they are after the origin operation to pad. Full diff: https://github.com/llvm/llvm-project/pull/165420.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Linalg/Transforms/PadTilingInterface.cpp b/mlir/lib/Dialect/Linalg/Transforms/PadTilingInterface.cpp
index 3e787a2ad0ef5..52ab92f180575 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/PadTilingInterface.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/PadTilingInterface.cpp
@@ -288,10 +288,6 @@ FailureOr<PadTilingInterfaceResult> linalg::rewriteAsPaddedOp(
return failure();
}
- OpBuilder::InsertionGuard g(builder);
- // Set IP after toPad because we also take the dims of toPad's output.
- builder.setInsertionPointAfter(toPad);
-
// 1. Get the loopUpperBounds from the TilingInterface.
SmallVector<Range> iterationDomain = toPad.getIterationDomain(builder);
|
fabianmcg
left a comment
fabianmcg
left a comment
There was a problem hiding this comment.
The expectation that the caller is the one setting the insertion point is already how things like rewrite patterns work, so conceptually LGTM.
Please add a comment documenting this is the expectation, also there's a build error because there's a transform not setting the insertion point.
Done, I'm guessing since you only had minor comments, it's okay to land. If there are any further comments, happy to address them in a post commit review. |
…vm#165420) Remove insertion point in rewriteAsPaddedOp. There is no gurantee that the sizes provided by the user are before the operation to pad. It's better to let the user handle where to insert the newly created operations, as long as they are after the origin operation to pad.
…vm#165420) Remove insertion point in rewriteAsPaddedOp. There is no gurantee that the sizes provided by the user are before the operation to pad. It's better to let the user handle where to insert the newly created operations, as long as they are after the origin operation to pad.
4 participants
Remove insertion point in rewriteAsPaddedOp. There is no gurantee that the sizes provided by the user are before the operation to pad. It's better to let the user handle where to insert the newly created operations, as long as they are after the origin operation to pad.