This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Lower linalg to affine loops
ClosedPublic

Authored by asaadaldien on Jan 2 2020, 10:06 AM.

Diff Detail

Event Timeline

asaadaldien created this revision.Jan 2 2020, 10:06 AM
nicolasvasilache accepted this revision.Jan 2 2020, 10:24 AM

Looks great and is as simple as it should be, thanks!
Accepting conditioned on the nit above.

cc @ftynse @AlexEichenberger @mravishankar

mlir/lib/Dialect/Linalg/Utils/Utils.cpp
114

please fold these 3 defs into their uses

This revision is now accepted and ready to land.Jan 2 2020, 10:24 AM
rriddle added inline comments.Jan 2 2020, 10:37 AM
mlir/include/mlir/Dialect/Linalg/Utils/Utils.h
12

Why is this header necessary?

mlir/lib/Dialect/Linalg/Utils/Utils.cpp
25

Is this really necessary here?

112

const auto -> Value

Resolve comments...

Unit tests: pass. 61146 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

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

lint fixes...

Unit tests: pass. 61146 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

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

ftynse added inline comments.Jan 2 2020, 11:48 PM
mlir/include/mlir/Dialect/Linalg/Utils/Utils.h
69

MLIR uses /// for class documentation

mlir/lib/Dialect/Linalg/Utils/Utils.cpp
113

Does something guarantee that there is a defining op?

mravishankar added inline comments.Jan 2 2020, 11:58 PM
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
113

You can probably change emitLoopRanges that generates the ranges argument at the called of this function to return Operation * instead of Value.

114

Nit: I think you can use lbs.emplace_back(rangeOp.min()). Same below

Resolve comments

asaadaldien marked 5 inline comments as done.Jan 3 2020, 9:32 AM
asaadaldien added inline comments.
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
113

Added assertions for the underlying type

113

We can keep it as ranges to have now to keep interface same as underlying RangeBuilders. We can revisit using ranges here in a different diff.

asaadaldien marked an inline comment as done.Jan 3 2020, 9:33 AM
asaadaldien added inline comments.
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
113

We can keep it as ranges for now to keep interface same as underlying RangeBuilders.
We can revisit using ranges here in a different diff.

Unit tests: pass. 61146 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

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

applying clang-format

Unit tests: pass. 61146 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

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

This revision was automatically updated to reflect the committed changes.

This seems to break on GCC 5.5:

mlir/mlir-core/mlir/lib/Dialect/Linalg/Utils/Utils.cpp:102:62: error: specialization of 'template<class LoopTy> mlir::edsc::GenericLoopNestRangeBuilder<LoopTy>::GenericLoopNestRangeBuilder(llvm::ArrayRef<mlir::edsc::ValueHandle*>, llvm::ArrayRef<mlir::Value>)' in different namespace [-fpermissive]
     ArrayRef<edsc::ValueHandle *> ivs, ArrayRef<Value> ranges) {
                                                              ^
mlir/mlir-core/mlir/lib/Dialect/Linalg/Utils/Utils.cpp:13:0:
/var/lib/buildkite-agent/builds/buildkite-6dd7db7bf5-48dzm-1/mlir/mlir-core/mlir/include/mlir/Dialect/Linalg/Utils/Utils.h:72:3: error:   from definition of 'template<class LoopTy> mlir::edsc::GenericLoopNestRangeBuilder<LoopTy>::GenericLoopNestRangeBuilder(llvm::ArrayRef<mlir::edsc::ValueHandle*>, llvm::ArrayRef<mlir::Value>)' [-fpermissive]
   GenericLoopNestRangeBuilder(ArrayRef<edsc::ValueHandle *> ivs,
   ^
mlir/mlir-core/mlir/lib/Dialect/Linalg/Utils/Utils.cpp:108:56: error: specialization of 'template<class LoopTy> mlir::edsc::GenericLoopNestRangeBuilder<LoopTy>::GenericLoopNestRangeBuilder(llvm::ArrayRef<mlir::edsc::ValueHandle*>, llvm::ArrayRef<mlir::Value>)' in different namespace [-fpermissive]
     ArrayRef<ValueHandle *> ivs, ArrayRef<Value> ranges) {
                                                        ^
mlir/mlir-core/mlir/lib/Dialect/Linalg/Utils/Utils.cpp:13:0:
/var/lib/buildkite-agent/builds/buildkite-6dd7db7bf5-48dzm-1/mlir/mlir-core/mlir/include/mlir/Dialect/Linalg/Utils/Utils.h:72:3: error:   from definition of 'template<class LoopTy> mlir::edsc::GenericLoopNestRangeBuilder<LoopTy>::GenericLoopNestRangeBuilder(llvm::ArrayRef<mlir::edsc::ValueHandle*>, llvm::ArrayRef<mlir::Value>)' [-fpermissive]
   GenericLoopNestRangeBuilder(ArrayRef<edsc::ValueHandle *> ivs,
   ^