Page MenuHomePhabricator

[mlir] Add microbenchmark for linalg+async-parallel-for
ClosedPublic

Authored by ezhulenev on Fri, Nov 20, 2:45 PM.

Diff Detail

Event Timeline

ezhulenev created this revision.Fri, Nov 20, 2:45 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
ezhulenev requested review of this revision.Fri, Nov 20, 2:45 PM

Nice to see this in action! Right in time also for some parallel work in the sparse compiler!

mlir/integration_test/Dialect/Async/CPU/microbench-linalg-async-parallel-for.mlir
76

Nit: if you pick another name like "SERIAL" for the sequential case, the output nicely lines up

mlir/lib/ExecutionEngine/AsyncRuntime.cpp
234–235

I am guessing it is okay that merely pulling in the async runtime lib also implies starting a threadpool?

Are the changes in mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp bug fixes? Can you add IR tests for this (and commit separately so there is a proper commit description)?

mlir/integration_test/Dialect/Async/CPU/microbench-linalg-async-parallel-for.mlir
76

Aren't they identical? Can't we just use "CHECK" and have one?

mlir/lib/ExecutionEngine/AsyncRuntime.cpp
234–235

I'd expect this to only start the thread pool if there is something to execute (when this function is invoked for the first time)?

aartbik added inline comments.Fri, Nov 20, 6:09 PM
mlir/integration_test/Dialect/Async/CPU/microbench-linalg-async-parallel-for.mlir
76

Interesting. I may have set the wrong example in other test cases, where I duplicate the CHECK-XXX even if the cases are identical to make it easier to see which of the XXX variants was responsible for a failure. But I guess you could indeed use one CHECK and just look at the command line of the failing report.....

nicolasvasilache accepted this revision.Sat, Nov 21, 1:30 AM

Nice, hooray for composition :) !

This revision is now accepted and ready to land.Sat, Nov 21, 1:30 AM
ezhulenev updated this revision to Diff 306833.Sat, Nov 21, 3:24 AM
ezhulenev marked 5 inline comments as done.

Address PR comments

Will submit changes to AsyncToLLVM.cpp in a separate PR.

mlir/lib/ExecutionEngine/AsyncRuntime.cpp
234–235

Yes, this will be instantiated only of thread pool is required for async execution. Also this runtime is primarily for testing, e.g. in TF it will be replaced with a TFRT integration.

This revision was automatically updated to reflect the committed changes.

No, will take a look in ~30 min.

mehdi_amini added inline comments.Sat, Nov 21, 8:30 PM
mlir/lib/ExecutionEngine/AsyncRuntime.cpp
234–235

I believe that on the medium-to-long-term we will want a proper runtime here, in the scope of the low-level bits of TFRT. So I rather not consider this "just for testing" if it means that we're don't have a path to productionize this or if it becomes too hacky. This should all make sense and have an integration story other than TF/TFRT upstream.