[PATCH] D104516: [mlir] Add a ThreadPool to MLIRContext and refactor MLIR threading usage

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

In D104516#2832247 <https://reviews.llvm.org/D104516#2832247>, @lattner wrote:

>> It was mentioned in the other revision I believe, but ManagedStatic is what the LLVM threading utilities currently use. Moving to the context removed all of the environment related shutdowns that I was seeing.
>
> No, it isn't.  This is the problem:
>
>   Executor *Executor::getDefaultExecutor() {
>   // ...
>     static ManagedStatic<ThreadPoolExecutor, ThreadPoolExecutor::Creator,
>                          ThreadPoolExecutor::Deleter>
>         ManagedExec;
>     static std::unique_ptr<ThreadPoolExecutor> Exec(&(*ManagedExec));
>     return Exec.get();
>   }
>
> Note that "Exec" is not a managed static.  The "..." elided is some justification for this design that seems really dubious to me (but I admit I haven't mind melded with it!)

Right right, I'm only vaguely recalling watching the original review for this (D70447 <https://reviews.llvm.org/D70447>). Before that we effectively couldn't use the Parallel methods on windows.

> I mentioned this in the other phab thread: while I'm totally ok with this patch as a way to unblock progress, it really isn't the right thing. It doesn't make sense for MLIR to have its own threadpool tied to its context's. CPU resources aren't library specific, they are global.

I'm not completely against a static/global thread pool, but I would claim that a static/global thread pool is orthogonal to CPU resources being library specific. A lot of users today with the current LLVM global thread pool don't share threads with things non-LLVM (whether for good reasons or bad).
We've encountered a lot of "interesting" interactions surrounding threading and certain environments, and I want to say a lot of those are due to the way the current code is structured (which can be a bit obscure at times).  The weird threading environment interactions have been extremely taxing 
and difficult to debug, and often end with "I have no idea what's happening, guess I'll just disable multi-threading". I would strongly prefer we stay non-static until a proper solution can prove itself to be reliable enough. I don't know exactly what you've had in mind
for revamping the threading libraries, but I have a wish list if you are interested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104516/new/

https://reviews.llvm.org/D104516