Fix namespace for MLIR Async Runtime

[mjp41]

Summary

The MLIR Async runtime uses different namespacing for the header file, and the definitions of its C API. The header file places the extern "C" functions inside namespace mlir::runtime, and the definitions are not in a namespace. This causes issues in cl.exe. It treats the declaration and definition as different, and thus does not apply dllexport to the definition, which leads to the mlir_async_runtime.dll containing no definitions, and the mlir_async_runtime.lib not being generated.

This patch moves the namespace to cover the definitions, and thus generates the dll correctly on Windows with cl.exe.

This was tested with Visual Studio C++ 19.28.29336.

[mjp41]

The following minimised example shows the Visual Studio behaviour

C++
namespace foo
{
    extern "C" __declspec(dllexport) int f();
    extern "C" __declspec(dllexport) int g();
}

extern "C" int f()
{
    return 42;
}

namespace foo
{
    extern "C" int g()
    {
        return 42;
    }
}

Compiling this with cl.exe file.cc /LD will generate a DLL, but only containing g.

[mjp41]

Closing as code has changed.

[mjp41]

I am not sure what the correct approach to this should be given the new code for the JIT. Do you need to dllexport the __mlir_runner_init code, so that it appears in the DLL? Do you actually want the other symbols in the dll, or just use the export_symbol in the __mlir_runner_init?

Sorry, I am not familiar with the development here. I am just trying to unblock our Windows CI from taking a more up-to-date commit.

[rengolin]

This is the error:

CMake Error at D:/a/1/s/build/Release/mlir/install/lib/cmake/mlir/MLIRTargets.cmake:892 (message):
  The imported target "mlir_async_runtime" references the file

     "D:/a/1/s/build/Release/mlir/install/lib/mlir_async_runtime.lib"

  but this file does not exist.

From what I can see, there MLIRTargets has LLVMIR, and there's a conversion between Async to LLVM (with registration of all those functions declared in AsyncRuntime.h) which I'm assuming is why MLIRTargets needs the async runtime.

I don't understand enough about the Async runtime to assert that the LLVM IR target really needs it. It could be just a linking dependency, not a real code dependency.

Closing as code has changed.

I don't understand what you mean by that. Last change to this file is last year.

Also, you may have uploaded an unrelated change (clang format) to this review.

[ezhulenev]

Ideally the library should only export __mlir_runner_destroy symbol (see https://reviews.llvm.org/D94312). Unfortunately I don't have an easy access to Windows build env, so I didn't properly cleanup exports for all symbols in that patch.

[mjp41]

@ezhulenev I think I can see what the fix should be. Does the check-mlir cover this example? Or is there additional testing I should do?

[mjp41]

Updated to export the symbols as suggested by @ezhulenev.

[rengolin]

In D95386#2522648, @mjp41 wrote:

@ezhulenev I think I can see what the fix should be. Does the check-mlir cover this example? Or is there additional testing I should do?

It doesn't seem like check-mlir covers this case (or we'd see this failing, as there are windows-mlir builds), so if it Verona builds with it and nothing else breaks, it should be "ok". :)

My Windows DLL knowledge is not good enough to suggest a specific test for this, and if it was, I don't think check-mlir is the right place, as this is a system issue.

Perhaps, now that MLIR is in the monorepo, we could have MLIR execution tests in the test-suite, but that's certainly for another patch. :)

Regarding the implementation, this patch certainly seems cleaner than previously, so I'm happy with it, but @ezhulenev should check and approve.

[mjp41]

@rengolin, I manually checked the DLL symbols contained __mlir_runner_init and __mlir_runner_destroy and nothing else. But I didn't know a command line that I could use to test if this what the context for loading this was expecting.

...\llvm-project>dumpbin /exports ...\llvm-project\build-win\bin\mlir_async_runtime.dll
Microsoft (R) COFF/PE Dumper Version 14.28.29336.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file ...\llvm-project\build-win\bin\mlir_async_runtime.dll

File Type: DLL

  Section contains the following exports for mlir_async_runtime.dll

    00000000 characteristics
    FFFFFFFF time date stamp
        0.00 version
           1 ordinal base
           2 number of functions
           2 number of names

    ordinal hint RVA      name

          1    0 00009B8D __mlir_runner_destroy = @ILT+35720(__mlir_runner_destroy)
          2    1 0000641A __mlir_runner_init = @ILT+21525(__mlir_runner_init)

  Summary

        1000 .00cfg
        7000 .data
        1000 .didat
        4000 .idata
       25000 .pdata
       A8000 .rdata
        5000 .reloc
        1000 .rsrc
      218000 .text
        1000 .tls

Looking at the disassembly, the body __mlir_runner_init it is calling a lambda with pointers to the internal functions (e.g. mlirAsyncRuntimeDropRef), and those function exist inside the dll, but are not exported.

[theraven]

Assuming @ezhulenev's description of the desired behaviour is correct (which the comment on line 366 appears to support), this patch looks right. I don't think we have tests for the sets of exported symbols anywhere else, which is a bit unfortunate.

I'm not particularly happy about the definition of API here, we should have a generic LLVM macro for this, but we don't and that's a separate issue.

[ezhulenev]

Thanks for the cleanup!

check-mlir runs few async tests using this runtime API as a part of mlir-cpu-runner tests. Also there are few integration tests and benchmarks for async dialect:

-DLLVM_ENABLE_PROJECTS=mlir
-DLLVM_ENABLE_THREADS=ON 
-DMLIR_INCLUDE_INTEGRATION_TESTS=ON
-DBUILD_SHARED_LIBS=ON

[mjp41]

So -DBUILD_SHARED_LIBS=ON is not supported on Windows. I had the same failures with and without this patch for MLIR integration tests. So I have done as much as I can to know this is not going to regress anything.

[mehdi_amini]

This patch is fine, but I'd still be interested to understand if this could be made to show up during check-mlir with one of the Cmake configuration that can run on Windows?

[rengolin]

In D95386#2525931, @mehdi_amini wrote:

This patch is fine, but I'd still be interested to understand if this could be made to show up during check-mlir with one of the Cmake configuration that can run on Windows?

If that's possible, that'd be awesome, as we seem to have hit it twice already in our monthly LLVM update. :)

[mjp41]

So after a little fiddling, I manually ran a test:

> bin\mlir-opt.exe ..\mlir\integration_test\Dialect\Async\CPU\test-async-parallel-for-1d.mlir -async-parallel-for -async-ref-counting  -convert-async-to-llvm  -convert-scf-to-std  -convert-std-to-llvm | bin\mlir-cpu-runner.exe -e entry -entry-point-result=void -O0 -shared-libs=bin\mlir_async_runtime.dll -shared-libs=bin\mlir_runner_utils.dll | bin\FileCheck --dump-input=always ..\mlir\integration_test\Dialect\Async\CPU\test-async-parallel-for-1d.mlir

Input file: <stdin>
Check file: ..\mlir\integration_test\Dialect\Async\CPU\test-async-parallel-for-1d.mlir

-dump-input=help explains the following input dump.

Input was:
<<<<<<
   1: Unranked Memref base@ = 000001C41C5D9E60 rank = 1 offset = 0 sizes = [9] strides = [1] data =
   2: [0, 1, 2, 3, 4, 5, 6, 7, 8]
   3: Unranked Memref base@ = 000001C41C5D9E60 rank = 1 offset = 0 sizes = [9] strides = [1] data =
   4: [0, 0, 2, 0, 4, 0, 6, 0, 8]
   5: Unranked Memref base@ = 000001C41C5D9E60 rank = 1 offset = 0 sizes = [9] strides = [1] data =
   6: [-20, 0, 0, -17, 0, 0, -14, 0, 0]
>>>>>>

This seems to have worked correctly. The names of libraries needed the lib prefix dropping from the command line, e.g. -shared-libs=bin\mlir_async_runtime.dll. Also, two assertions were tripped in

...\llvm-project\llvm\lib\ExecutionEngine\Orc\Core.cpp

Lines 915-918

assert(Flags ==
           (SymI->second.getFlags() &
            ~(JITSymbolFlags::Weak | JITSymbolFlags::Common)) &&
    "Resolved flags should match the declared flags");

and Lines 2633-2634

assert((KV.second.getFlags() & ~WeakFlags) == (I->second & ~WeakFlags) &&
   "Resolving symbol with incorrect flags");

I locally disabled these to see to get the run above.

[mjp41]

Just to follow up. The tests for the ExecutionEngine as far as I can see do not run on Windows. Getting tests for the ExecutionEngine working on Windows seems like a larger undertaking, than I can currently do, and would require checking a lot of code I am not familiar with. My limited experiment seems to imply the changes I have made would help with making the MLIR execution engine work on Windows.

@rengolin, can you merge as is?

[rengolin]

In D95386#2536963, @mjp41 wrote:

@rengolin, can you merge as is?

I'm happy merging it as is. @mehdi_amini any further concerns?

[mehdi_amini]

LG, my previous comment was more aspirational, I didn't intend to hold this revision :)

[rengolin]

In D95386#2537038, @mehdi_amini wrote:

LG, my previous comment was more aspirational, I didn't intend to hold this revision :)

Awesome, thanks!

[stella.stamenova]

It looks like this broke the windows mlir bot. Please revert it or fix it ASAP:

http://lab.llvm.org:8011/#/builders/13/builds/4013

[rengolin]

In D95386#2537500, @stella.stamenova wrote:

It looks like this broke the windows mlir bot. Please revert it or fix it ASAP:

http://lab.llvm.org:8011/#/builders/13/builds/4013

It's a 2017 bug fixed in the 2019 version.

Sorry about that. We're working on a fix, thanks!

mlir/include/mlir/ExecutionEngine/AsyncRuntime.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_EXECUTIONENGINE_ASYNCRUNTIME_H_
 #define MLIR_EXECUTIONENGINE_ASYNCRUNTIME_H_
 
 #include <stdint.h>
 
-#ifdef _WIN32
-#ifndef MLIR_ASYNCRUNTIME_EXPORT
 #ifdef mlir_async_runtime_EXPORTS
 // We are building this library
-#define MLIR_ASYNCRUNTIME_EXPORT __declspec(dllexport)
 #define MLIR_ASYNCRUNTIME_DEFINE_FUNCTIONS
-#else
-// We are using this library
-#define MLIR_ASYNCRUNTIME_EXPORT __declspec(dllimport)
 #endif // mlir_async_runtime_EXPORTS
-#endif // MLIR_ASYNCRUNTIME_EXPORT
-#else
-#define MLIR_ASYNCRUNTIME_EXPORT
-#define MLIR_ASYNCRUNTIME_DEFINE_FUNCTIONS
-#endif // _WIN32
 
 namespace mlir {
 namespace runtime {
 
 //===----------------------------------------------------------------------===//
 // Async runtime API.
 //===----------------------------------------------------------------------===//
 

 using CoroHandle = void *;           // coroutine handle
 using CoroResume = void (*)(void *); // coroutine resume function
 
 // Async runtime uses reference counting to manage the lifetime of async values
 // (values of async types like tokens, values and groups).
 using RefCountedObjPtr = void *;
 
 // Adds references to reference counted runtime object.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-    mlirAsyncRuntimeAddRef(RefCountedObjPtr, int32_t);
+extern "C" void mlirAsyncRuntimeAddRef(RefCountedObjPtr, int32_t);
 
 // Drops references from reference counted runtime object.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-    mlirAsyncRuntimeDropRef(RefCountedObjPtr, int32_t);
+extern "C" void mlirAsyncRuntimeDropRef(RefCountedObjPtr, int32_t);
 
 // Create a new `async.token` in not-ready state.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT AsyncToken *mlirAsyncRuntimeCreateToken();
+extern "C" AsyncToken *mlirAsyncRuntimeCreateToken();
 
 // Create a new `async.value` in not-ready state. Size parameter specifies the
 // number of bytes that will be allocated for the async value storage. Storage
 // is owned by the `async.value` and deallocated when the async value is
 // destructed (reference count drops to zero).
-extern "C" MLIR_ASYNCRUNTIME_EXPORT AsyncValue *
-    mlirAsyncRuntimeCreateValue(int32_t);
+extern "C" AsyncValue *mlirAsyncRuntimeCreateValue(int32_t);
 
 // Create a new `async.group` in empty state.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT AsyncGroup *mlirAsyncRuntimeCreateGroup();
+extern "C" AsyncGroup *mlirAsyncRuntimeCreateGroup();
 
-extern "C" MLIR_ASYNCRUNTIME_EXPORT int64_t
-mlirAsyncRuntimeAddTokenToGroup(AsyncToken *, AsyncGroup *);
+extern "C" int64_t mlirAsyncRuntimeAddTokenToGroup(AsyncToken *, AsyncGroup *);
 
 // Switches `async.token` to ready state and runs all awaiters.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-mlirAsyncRuntimeEmplaceToken(AsyncToken *);
+extern "C" void mlirAsyncRuntimeEmplaceToken(AsyncToken *);
 
 // Switches `async.value` to ready state and runs all awaiters.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-mlirAsyncRuntimeEmplaceValue(AsyncValue *);
+extern "C" void mlirAsyncRuntimeEmplaceValue(AsyncValue *);
 
 // Blocks the caller thread until the token becomes ready.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-mlirAsyncRuntimeAwaitToken(AsyncToken *);
+extern "C" void mlirAsyncRuntimeAwaitToken(AsyncToken *);
 
 // Blocks the caller thread until the value becomes ready.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-mlirAsyncRuntimeAwaitValue(AsyncValue *);
+extern "C" void mlirAsyncRuntimeAwaitValue(AsyncValue *);
 
 // Blocks the caller thread until the elements in the group become ready.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-mlirAsyncRuntimeAwaitAllInGroup(AsyncGroup *);
+extern "C" void mlirAsyncRuntimeAwaitAllInGroup(AsyncGroup *);
 
 // Returns a pointer to the storage owned by the async value.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT ValueStorage
-mlirAsyncRuntimeGetValueStorage(AsyncValue *);
+extern "C" ValueStorage mlirAsyncRuntimeGetValueStorage(AsyncValue *);
 
 // Executes the task (coro handle + resume function) in one of the threads
 // managed by the runtime.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void mlirAsyncRuntimeExecute(CoroHandle,
-                                                                 CoroResume);
+extern "C" void mlirAsyncRuntimeExecute(CoroHandle, CoroResume);
 
 // Executes the task (coro handle + resume function) in one of the threads
 // managed by the runtime after the token becomes ready.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-mlirAsyncRuntimeAwaitTokenAndExecute(AsyncToken *, CoroHandle, CoroResume);
+extern "C" void mlirAsyncRuntimeAwaitTokenAndExecute(AsyncToken *, CoroHandle,
+                                                     CoroResume);
 
 // Executes the task (coro handle + resume function) in one of the threads
 // managed by the runtime after the value becomes ready.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
-mlirAsyncRuntimeAwaitValueAndExecute(AsyncValue *, CoroHandle, CoroResume);
+extern "C" void mlirAsyncRuntimeAwaitValueAndExecute(AsyncValue *, CoroHandle,
+                                                     CoroResume);
 
 // Executes the task (coro handle + resume function) in one of the threads
 // managed by the runtime after the all members of the group become ready.
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void
+extern "C" void
 mlirAsyncRuntimeAwaitAllInGroupAndExecute(AsyncGroup *, CoroHandle, CoroResume);
 
 //===----------------------------------------------------------------------===//
 // Small async runtime support library for testing.
 //===----------------------------------------------------------------------===//
 
-extern "C" MLIR_ASYNCRUNTIME_EXPORT void mlirAsyncRuntimePrintCurrentThreadId();
+extern "C" void mlirAsyncRuntimePrintCurrentThreadId();
 
 } // namespace runtime
 } // namespace mlir
 
 #endif // MLIR_EXECUTIONENGINE_ASYNCRUNTIME_H_

mlir/lib/ExecutionEngine/AsyncRuntime.cpp

struct AsyncGroup : public RefCounted {
   std::atomic<int> rank;
 
   // Pending awaiters are guarded by a mutex.
   std::mutex mu;
   std::condition_variable cv;
   std::vector<std::function<void()>> awaiters;
 };
 
-} // namespace runtime
-} // namespace mlir
 
 // Adds references to reference counted runtime object.
 extern "C" void mlirAsyncRuntimeAddRef(RefCountedObjPtr ptr, int32_t count) {
   RefCounted *refCounted = static_cast<RefCounted *>(ptr);
   refCounted->addRef(count);
 }
 
 // Drops references from reference counted runtime object.
extern "C" void mlirAsyncRuntimePrintCurrentThreadId() {
   std::cout << "Current thread id: " << thisId << std::endl;
 }
 
 //===----------------------------------------------------------------------===//
 // MLIR Runner (JitRunner) dynamic library integration.
 //===----------------------------------------------------------------------===//
 
 // Export symbols for the MLIR runner integration. All other symbols are hidden.
-#ifndef _WIN32
+#ifdef _WIN32
+#define API __declspec(dllexport)
+#else
 #define API __attribute__((visibility("default")))
+#endif
 
 extern "C" API void __mlir_runner_init(llvm::StringMap<void *> &exportSymbols) {
   auto exportSymbol = [&](llvm::StringRef name, auto ptr) {
     assert(exportSymbols.count(name) == 0 && "symbol already exists");
     exportSymbols[name] = reinterpret_cast<void *>(ptr);
   };
 
   exportSymbol("mlirAsyncRuntimeAddRef",
extern "C" API void __mlir_runner_init(llvm::StringMap<void *> &exportSymbols) {
   exportSymbol("mlirAsyncRuntimeAwaitAllInGroupAndExecute",
                &mlir::runtime::mlirAsyncRuntimeAwaitAllInGroupAndExecute);
   exportSymbol("mlirAsyncRuntimePrintCurrentThreadId",
                &mlir::runtime::mlirAsyncRuntimePrintCurrentThreadId);
 }
 
 extern "C" API void __mlir_runner_destroy() { resetDefaultAsyncRuntime(); }
 
-#endif // _WIN32
+} // namespace runtime
+} // namespace mlir
 
 #endif // MLIR_ASYNCRUNTIME_DEFINE_FUNCTIONS