Page MenuHomePhabricator

Avoid creating a ThreadPool in MlirOptMain when `--mlir-disable-threading` option is set
ClosedPublic

Authored by mehdi_amini on Jan 7 2022, 5:44 PM.

Details

Summary

a32300a changed it to create a ThreadPool eagerly so that it gets reused
across buffers, however it also made it so that we create a ThreadPool
early even if we're not gonna use it later because of the command line
option --mlir-disable-threading is provided.

Fix #53056

Diff Detail

Event Timeline

mehdi_amini created this revision.Jan 7 2022, 5:44 PM
mehdi_amini requested review of this revision.Jan 7 2022, 5:44 PM
rriddle added inline comments.Jan 7 2022, 5:47 PM
mlir/lib/Support/MlirOptMain.cpp
149–155

This feels not ideal, is there a way to check statically that threading is enabled? I thought we had something on MLIRContext for that. If not, could we add it?

mehdi_amini added inline comments.Jan 7 2022, 5:59 PM
mlir/lib/Support/MlirOptMain.cpp
149–155

By "statically" you mean "without an MLIRContext instance"?

We don't have this I believe. The use of --mlir-disable-threading is private to the context implementation and not exposed publicly right now.

jdd added a subscriber: jdd.Jan 7 2022, 6:00 PM
jdd added inline comments.
mlir/lib/Support/MlirOptMain.cpp
149–155

There is isThreadingGloballyDisabled() in MLIRContext.cpp. You could make that public and use it.

rriddle added inline comments.Jan 7 2022, 6:03 PM
mlir/lib/Support/MlirOptMain.cpp
149–155

Ah, I was thinking of isThreadingGloballyDisabled in MLIRContext.cpp. Maybe we could expose that in some way?

mehdi_amini added inline comments.Jan 7 2022, 6:06 PM
mlir/lib/Support/MlirOptMain.cpp
149–155

Well, of course, but I was a bit reluctant to make public the access to cl::opt things that I was seeing as conceptually private :)

rriddle accepted this revision.Jan 7 2022, 6:08 PM

LGTM in general, if we intend to do this more than once (either in another tool, or in user code) would be nice to expose something general for it.

mlir/lib/Support/MlirOptMain.cpp
149–155

True true, this just seemed like something we might need to do elsewhere. Either way, what you have is fine for local needs.

This revision is now accepted and ready to land.Jan 7 2022, 6:08 PM
jdd accepted this revision.Jan 7 2022, 6:09 PM
This revision was landed with ongoing or failed builds.Jan 7 2022, 6:18 PM
This revision was automatically updated to reflect the committed changes.