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

[Chris Lattner via Phabricator via llvm-commits]

lattner added a comment.

> 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!)



================
Comment at: mlir/test/Dialect/Affine/SuperVectorize/compose_maps.mlir:1
-// RUN: mlir-opt -allow-unregistered-dialect %s -affine-super-vectorizer-test -compose-maps 2>&1 |  FileCheck %s
+// RUN: mlir-opt -allow-unregistered-dialect %s -affine-super-vectorizer-test -compose-maps -mlir-disable-threading 2>&1 |  FileCheck %s
 
----------------
rriddle wrote:
> lattner wrote:
> > Why do these need to disable threading?
> Several existing tests depended on the previous threading behavior to work, mostly getting lucky that the `errs()` output was laid out in the way they expect. Switched to no longer disable threading, but adding split file markers to separate tests.
Ok for this patch, but gross.  Plz file a bug about these.  It is a bad antipattern to emit to `errs()`, these tests should be emitting errors so they are pinned to a location correctly (similar to the dependence analysis tests)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104516