This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Improve codegen strategy
ClosedPublic

Authored by nicolasvasilache on Jan 27 2021, 7:53 AM.

Details

Summary

This revision improves the usage of the codegen strategy by adding a few flags that
make it easier to control for the CLI.
Usage of ModuleOp is replaced by FuncOp as this created issues in multi-threaded mode.

A simple benchmarking capability is added for linalg.matmul as well as linalg.matmul_column_major.
This latter op is also added to linalg.

Now obsolete linalg integration tests that also take too long are deleted.

Correctness checks are still missing at this point.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Jan 27 2021, 7:53 AM
This revision is now accepted and ready to land.Jan 27 2021, 12:44 PM
aartbik accepted this revision.Jan 27 2021, 5:24 PM

Thanks for migrating more of these into integration test. Two nits, but good to go.

mlir/integration_test/Dialect/Linalg/CPU/benchmark_matmul.mlir
12

do we want to keep this in our tests?

87

here and below, can we have some basic CHECKs just so that this is not just a benchmark but a test as well?

nicolasvasilache marked an inline comment as done.

Add some checks.

nicolasvasilache marked an inline comment as done.Jan 28 2021, 1:31 AM
nicolasvasilache added inline comments.
mlir/integration_test/Dialect/Linalg/CPU/benchmark_matmul.mlir
12

I would err towards the side of yes: it is become more and more obvious every day that MLIR does not come with batteries included and the mere finding of this option takes me 5 minutes every time. Can't even imagine what it is like for folks who do not know this option exists.
I would claim it is the same thing for:

// TODO: activate manually for now.
// attributes { passthrough = [["target-cpu", "skylake-avx512"], ["prefer-vector-width", "512"]]}

It's good to have minimal unit tests in /test but at some point the lack of usability of MLIR greatly outruns a few well placed comments.

87

SG.
Note I want to still be able to run the test print without modifying the file so I used tee to also redirect things out to stderr.
Will wait for the tests to run see if that causes concerns.

This revision was automatically updated to reflect the committed changes.
nicolasvasilache marked an inline comment as done.
mlir/test/mlir-cpu-runner/include/mlir_test_cblas_interface.h