Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
800 | Nit: drop trivial braces. | |
818 | Please avoid magic numbers. | |
821 | const & | |
829 | TypeRange should be implicitly constructible from a single type. | |
838–839 | .getResult(0) looks shorter. | |
842–853 | 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. |
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 | ||
800 | For numWorkerThreads == 0 this pass should be no-op, otherwise I assume it will crash with some division by zero | |
809 | Can you send this change in a separate PR, not to mix it with straightforward async runtime change | |
809 | Ah, just realized that numWorkerThreads is no longer a constant. |
mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp | ||
---|---|---|
821 | 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 |
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 | ||
800 | 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. | |
808 | Done. | |
809 | I'm assuming this is withdrawn. | |
817 | 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. | |
821 | Great! | |
829 | Obsolete. | |
838–839 | Obsolete. | |
842–853 | Done. |
mlir/lib/Dialect/Async/Transforms/AsyncParallelFor.cpp | ||
---|---|---|
822 | 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
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
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.