This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Fix Linalg EDSC builders
ClosedPublic

Authored by nicolasvasilache on Jan 15 2020, 9:24 AM.

Details

Summary

This diff fixes the fact that the method mlir::edsc::makeGenericLinalgOp
incorrectly adds 2 blocks to Linalg ops.

Tests are updated accordingly.

Diff Detail

Event Timeline

Unit tests: pass. 61804 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

asaadaldien accepted this revision.Jan 15 2020, 11:24 AM

LGTM! an inline question trying to understand the Builder blocks scoping here.

mlir/lib/Dialect/Linalg/EDSC/Builders.cpp
97
OpBuilder bb(region);
103

Does the builder create a new block instead of mutating region.begin that we have to clean up ?

This revision is now accepted and ready to land.Jan 15 2020, 11:24 AM
nicolasvasilache marked an inline comment as done.Jan 15 2020, 12:59 PM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/EDSC/Builders.cpp
103

Yes, currently it does.
I can improve this by extending ScopedContext slightly I think.
This is because the mechanism predates regions and has not yet been updated.

Address review comments.

This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 61912 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

nicolasvasilache marked 2 inline comments as done.Jan 16 2020, 7:26 AM
asaadaldien added inline comments.Jan 16 2020, 9:36 AM
mlir/lib/EDSC/Builders.cpp
307 ↗(On Diff #238493)

typo s/BockBuilder/BlockBuilder