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

Mon Jun 21 18:40:15 PDT 2021

rriddle added a comment.

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

> did you consider shoving the existing threadpool stuff into a `ManagedStatic`?  That is how we typically handle problems like this, they are destroyed on llvm_shutdown instead of at global deinit.

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.

================
Comment at: mlir/include/mlir/IR/ThreadingUtilities.h:1
+//===- ThreadingUtilities.h - MLIR Threading Utilities ----------*- C++ -*-===//
+//
----------------
lattner wrote:
> I'd recommend calling this just "Threading.h".
> 
> This also points out a layering issue: MLIRContext is the right place for us to scope this (so I'm ok with the patch), but this really has nothing to do with the IR!
> 
> One solution would be to split MLIRContext, give it a subclass like "MLIRNonIRContext" (better names welcome) that contain the thread pool, diagnostics utility and other stuff that would be generic, and put it in mlir/Support.  MLIRContext would derive from it, and would remain the currency type, but generic utilities like this would take "MLIRNonIRContext".
> 
> Anyway, discussion for another day, just saying that general parallelism stuff would be better in Support.
I think the major sticking point is what to do about `Location`, which is used pervasively by Diagnostics. Most of the handlers interact with the location types, so those I suppose could stay in IR, but Diagnostic::getLocation would have to hold/return something like `RawLocation`? Which would have to store something opaque. I think it's doable, but the layering would have to be carefully done such that interacting with Diagnostics doesn't become ugly.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104516