Skip to content

[mlir][linalg] Edit the yieldOutputs method's builder #73900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[mlir][linalg] Edit the yieldOutputs method's builder #73900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,8 +480,8 @@ class RegionBuilderHelper {
llvm_unreachable("unsupported type conversion function");
}

void yieldOutputs(ValueRange values) {
OpBuilder builder = getBuilder();
void yieldOutputs(OpBuilder builder, ValueRange values) {
builder.setInsertionPointToEnd(&block);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference here exactly with before? Looking at the implementation of getBuilder() I'm not spotting it immediately.

Copy link
Contributor Author

@amirBish amirBish Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous one the builder is being constructed from the block's argument context block.getArgument(0).getContext() , here is not the case It is being passed as is.
when using the rewriter of the MatchAndRewrite pattern we should use the rewriter which has been passed and not creating a new builder. That what has made the memory leakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping

Location loc = builder.getUnknownLoc();
builder.create<YieldOp>(loc, values);
}
Expand Down
1 change: 1 addition & 0 deletions mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ struct SparseTensorCodegenPass
// The following operations and dialects may be introduced by the
// codegen rules, and are therefore marked as legal.
target.addLegalOp<linalg::FillOp>();
target.addLegalOp<linalg::YieldOp>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a test failing if you don't add that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, without this legalizing YieldOp, the RewritePattern won't be applied (since YieldOp it's not legal).

target.addLegalDialect<
arith::ArithDialect, bufferization::BufferizationDialect,
complex::ComplexDialect, memref::MemRefDialect, scf::SCFDialect>();
Expand Down
2 changes: 1 addition & 1 deletion mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-yaml-gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,7 @@ void {0}::regionBuilder(ImplicitLocOpBuilder &b,
SmallVector<Value> yields;
{2}
{3}
helper.yieldOutputs(yields);
helper.yieldOutputs(b, yields);
}
)FMT";
auto &args = opConfig.structuredOp->args;
Expand Down