[PATCH] D51052: [WIP] LLJITTest

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Maybe it makes sense in oder to illustrate the usage of your API, to give people a very simple example (like `UnthreadedSingleDylib`) and extended it step by step to more advanced ones (like `ThreadedMultiDylib`).

I don't see how to avoid the `PassManagerRunMutex` in `IRCompileLayer2::emit()`:

- I tried to add mutexes in MCAssembler, TargetLoweringObjectFile, AtomicExpandPass, etc., but it got very complicated very quickly. Everything goes through `PassManager::run()`, but a mutex there would affect a lot of other code. `IRCompileLayer2` appeared to be a good point.
- I always thought that support for multithreaded codegen is implicitly given when using different `LLVMContext` instances. It doesn't look like that here. Maybe there's more requirements to it?



================
Comment at: unittests/ExecutionEngine/Orc/LLJITTest.cpp:55
   }
 );
 
----------------
Changed example functions: foo and bar are disjunct now, buz is the one that depends on both of them.


================
Comment at: unittests/ExecutionEngine/Orc/LLJITTest.cpp:161
         assert(Ms.size() < MaxMaterializerThreads &&
                "More slots used than preallocated");
 
----------------
The thread that runs `DispatchMaterialization` can't be blocked, right? So it must spawn a new thread, which must be joined back to this thread later. How do we know the correct one? Track it in `MaterializationUnit` and join in destructor? Also see my below comments.


================
Comment at: unittests/ExecutionEngine/Orc/LLJITTest.cpp:184
     JITEvaluatedSymbol FooSym = cantFail(Jit->lookup("foo"));
     EXPECT_EQ(1, Exec(FooSym)); // Automatically waits for materializer.
   });
----------------
Can't join the `DispatchMaterialization` thread here, because I don't know whether it's `M[0]` or `M[1]`.


================
Comment at: unittests/ExecutionEngine/Orc/LLJITTest.cpp:190
     EXPECT_EQ(1, Exec(BarSym)); // Automatically waits for materializer.
   });
   
----------------
Concurrent lookup for `foo` and `bar` works due to the mutex in `IRCompileLayer2::emit()`.


================
Comment at: unittests/ExecutionEngine/Orc/LLJITTest.cpp:194
   JoinAll(Ts);
-  JoinAll(Ms);
+  JoinAll(Ms); // This should happen in the above runner threads.
 
----------------
This randomly causes failures. I think threads should always be joined in the thread that they were forked from. Not simple, even with a fixed-size thread pool.


================
Comment at: unittests/ExecutionEngine/Orc/LLJITTest.cpp:269
   
-  // FIXME: Lookup for "buz" can't run in parallel.
-  JoinAll(Ts);
-  JoinAll(Ms);
-
-  // Allow buz to resolve foo and bar from the other dylibs.
-  Extra2.addToSearchOrder(Main);
-  Extra2.addToSearchOrder(Extra1);
-
+  // Lookup for "buz" works in parallel here even without a barrier.
   Ts.emplace_back([&]() {
----------------
No barrier with `JoinAll` necessary here. Maybe because `foo` and `bar` are in other dylibs?


Repository:
  rL LLVM

https://reviews.llvm.org/D51052