Page MenuHomePhabricator

[async] Get the number of worker threads from the runtime.
ClosedPublic

Authored by bakhtiyarneyman on Jan 19 2022, 11:03 PM.

Diff Detail

Event Timeline

bakhtiyarneyman requested review of this revision.Jan 19 2022, 11:03 PM
ftynse requested changes to this revision.Jan 20 2022, 12:51 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Async/IR/AsyncOps.td
543

Why is this necessary? The return type is always index, ODS should not how to build it. If something is not working without this, it is probably a missing ODS functionality, we shouldn't need to write such boilerplate.

544–547

Nit: put double indentation for function arguments, otherwise it's hard to differentiate them from the body.

mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
804

Nit: drop trivial braces.

822

Please avoid magic numbers.

825

const &

833

TypeRange should be implicitly constructible from a single type.

842–843

.getResult(0) looks shorter.

846–857

Please avoid nested calls to Builder::create. Beyond being harder to read, it leads to a platform-dependent order of operations since the order of evaluation for function call arguments is implementation-defined in C++. I suspect this is the reason why the test is broken on Windows (makeSwitch creates operations and so does the other argument of MulFOp).

mlir/test/Dialect/Async/async-parallel-for-num-worker-threads.mlir
13

Nit: why do we even emit this conditional if we know the condition?

16

Please avoid matching SSA value names, even for constants.

This revision now requires changes to proceed.Jan 20 2022, 12:51 AM
ezhulenev added inline comments.Jan 20 2022, 12:15 PM
mlir/include/mlir/Dialect/Async/IR/AsyncOps.td
541

I'd prefer to see : type($result) here, even if ODS can infer it, explicit typing makes it easier to read, and if ODS can't do it now then inferReturnTypes is too much.

mlir/include/mlir/ExecutionEngine/AsyncRuntime.h
129

nit: empty line before the next comment

mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
804

For numWorkerThreads == 0 this pass should be no-op, otherwise I assume it will crash with some division by zero

813

Can you send this change in a separate PR, not to mix it with straightforward async runtime change

813

Ah, just realized that numWorkerThreads is no longer a constant.

ezhulenev added inline comments.Jan 20 2022, 12:20 PM
mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
812

Can you include simplified version of the old code in comments, because parsing what's going on here from builders is too hard.

821

Here and in all other labmdas capture can be just [&] because they do not leave the scope.

ezhulenev added inline comments.Jan 20 2022, 12:37 PM
mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
825

Instead of generating nested scf.if you can do that with a short number of cmpi and select, I think it will be easier to match in tests and read in mlir dumps, and no need to to recursive builders, just a loop over brackets will do the work

%threads = async.runtime.num_threads

%init = constant 8.0 : float // (0, 4]

%p4 = cmpi sgt %threads %c4     // num_threads > 4
%p8 = cmpi sgt %threads %c8     // num_threads > 8
%p16 = cmpi sgt %threads %c16 // num_threads > 16
%p32 = cmpi sgt %threads %c32 // num_threads > 32
%p64 = cmpi sgt %threads %c64 // num_threads > 64

%s0 = select %p4   %c4.0  %c8.0
%s1 = select %p8   %c2.0  %s0
%s2 = select %p16 %c1.0  %s1
%s3 = select %p32 %c0.8  %s2
%scale = select %p64 %c0.6  %s3
bakhtiyarneyman marked 18 inline comments as done.

Address comments.

Revert changes to Bazel file because it doesn't belong in this CL.

mlir/include/mlir/Dialect/Async/IR/AsyncOps.td
541

Done.

543

I was confused about interfaces: I didn't realize that merely importing the interface module is sufficient for it to take effect. I thought that is is necessary to use it as a trait on an op.

Removed.

544–547

Obsolete.

mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
804

Definition of maxComputeBlock prevents that. I'm not changing logic in this regard. I suppose an early exit is possible, but I'd argue it belongs in a separate PR.

812

Done.

813

I'm assuming this is withdrawn.

821

Obsolete. FWIW, makeSwitch needed to be passed by copy, because it was supposed to replace past self, so I opted for maximum explicitness. But it's moot point now.

825

Great!

833

Obsolete.

842–843

Obsolete.

846–857

Done.

ezhulenev added inline comments.Jan 21 2022, 6:24 AM
mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp
826

You can include "mlir/Support/LLVM.h" to bring APFloat into mlir namespace (for consistency with other mlir files), also a separate makeFloat doesn't make much sense now, its used just twice, and "creating custom mini DSLs" is frowned upon in MLIR

mlir/lib/ExecutionEngine/AsyncRuntime.cpp
441
auto *runtime = getDefaultAsyncRuntime();
  runtime->getThreadPool()-> <get the real number of worker threads> ``` (snippet from `mlirAsyncRuntimeExecute`

Can you please also add tests cases to tests (benchmarks?) in this folder: https://github.com/llvm/llvm-project/tree/main/mlir/test/Integration/Dialect/Async/CPU

bakhtiyarneyman marked 2 inline comments as done.

Address comments.

ezhulenev accepted this revision.Jan 26 2022, 11:54 AM

I get this error locally:

AILED: lib/libMLIRAsync.so.14git
: && /usr/bin/c++ -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -g  -Wl,-z,defs -Wl,-z,nodelete   -Wl,-rpath-link,/usr/local/google/home/ezhulenev/projects/llvm-project/build/./lib -shared -Wl,-soname,libMLIRAsync.so.14git -o lib/libMLIRAsync.so.14git tools/mlir/lib/Dialect/Async/IR/CMakeFiles/obj.MLIRAsync.dir/Async.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libMLIRDialect.so.14git  lib/libMLIRIR.so.14git  lib/libMLIRSupport.so.14git  -lpthread  lib/libLLVMSupport.so.14git  -Wl,-rpath-link,/usr/local/google/home/ezhulenev/projects/llvm-project/build/lib && :
/usr/bin/ld: tools/mlir/lib/Dialect/Async/IR/CMakeFiles/obj.MLIRAsync.dir/Async.cpp.o: in function `mlir::detail::InferTypeOpInterfaceTrait<mlir::async::AddToGroupOp>::verifyTrait(mlir::Operation*)':
/usr/local/google/home/ezhulenev/projects/llvm-project/build/tools/mlir/include/mlir/Interfaces/InferTypeOpInterface.h.inc:141: undefined reference to `mlir::detail::verifyInferredResultTypes(mlir::Operation*)'

I think CMakeLists has to be updated to be consistent with BUILD.bazel, though it seems that InferTypeOpInterface is no longer used and can be deleted

Update relevant CMakeList.txt.

ezhulenev accepted this revision.Jan 31 2022, 12:05 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 31 2022, 12:06 PM
This revision was automatically updated to reflect the committed changes.