[PATCH] D79266: [mlir] Add a new context flag for disabling/enabling multi-threading

[Chris Lattner via Phabricator via llvm-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

Awesome, thank you for tackling this River, I added some suggestions above.

When this lands, I'll let you know what speedup I see on my example (and see if anything else should be guarded by this).  Thank you again for working on this!



================
Comment at: mlir/docs/PassManagement.md:804
 ```shell
-$ mlir-opt foo.mlir -disable-pass-threading -pass-pipeline='func(cse,canonicalize)' -convert-std-to-llvm -pass-timing -pass-timing-display=list
+$ mlir-opt foo.mlir -mlir-disable-threading -pass-pipeline='func(cse,canonicalize)' -convert-std-to-llvm -pass-timing -pass-timing-display=list
 
----------------
Bikeshed: I'm not sure the precedence on this, I think that "-disable-mlir-threading" would read better and be more discoverable than "-mlir-disable-threading".


================
Comment at: mlir/lib/IR/MLIRContext.cpp:55
+      "mlir-disable-threading",
+      llvm::cl::desc("Disabling multi-threading in the MLIRContext")};
+
----------------
When this shows up in mlir-opt etc, I think this is effectively "Disabling multi-threading in MLIR" - right?  The passmgr and everything else that would create threads should be gated on the MLIRContext bit this sets.


================
Comment at: mlir/lib/IR/MLIRContext.cpp:156
+  // Check for an existing instance in read-only mode.
+  if (!threadingIsDisabled) {
     llvm::sys::SmartScopedReader<true> instanceLock(mutex);
----------------
Maybe it is just the way my brain works, but I'd recommend changing the internal flag (and the parameters that pass it around) to "threadingIsEnabled" to make the conditions easier to read.

You can still make it default to the same behavior of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79266