-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CIR][TargetLowering][NFC] Refactor LowerModule to a unified global state #759
Conversation
namespace mlir { | ||
namespace cir { | ||
|
||
void LowerModuleRegistry::initializeWithModule(ModuleOp module) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrated from the removed function createLowerModule
.
|
||
/// Registry for the LowerModule, enabling easy access to the LowerModule from | ||
/// various libraries. | ||
class LowerModuleRegistry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the naming here. Advice welcome.
Two high-level questions:
|
No. Because I think it generally makes no sense to support "initialize a global state with multi threading". The caller should avoid that kind of abuse😂. For implicit multi threading, MLIR multi threading generally happens in
I think possibly Yes for the future, but Not Yet for now (?). Do we have a way to explicitly reject such situation? Maybe add an |
Let me explain why we need such a (seemingly hacky) global state rather than passing the instance around. Intrusive changesWe have 3 producers ( Implementation hidingOur goal is to make For such problem, we have to either:
And the container of LowerModule is exactly the same thing as the registry, except that the latter also doesn't need a bunch of intrusive changes. Precedent in MLIR of global state
This is just a simple direct approach to improve the current LowerModule initialization. It has the following drawbacks:
That's my food for better ideas, suggestions welcome and appreciated! |
Hi, thanks for improving this and for the detailed explanation. The managed static also makes me a bit scared. I was thinking something way more direct:
|
I think I had a patch of ~600 lines roughly implementing it. The patch outlines the long call stack... Is it viable? |
Before you write more code, let's discuss a bit more.
I'm not sure I understand this situation, can you elaborate on why can't |
When a pass pipeline constructed by (Of course, CallConvLowering is not yet registered for cir-opt for now.) |
I changed my mind here, as long as we keep holding one per module the price shouldn't be too high - the complexity tradeoff doesn't seem worth it. Thanks for putting the work on putting this PR, let's followup on your other work. We can revisit this if performance become an issue. |
We expect the
LowerModule
to be available most of the time onceCIRGen
is complete, providing various support for lowering passes, especially target-specific information. To maintain a consistent state along the call tree and across all libraries, we should add a non-intrusive singleton class.This PR creates the singleton class
LowerModuleRegistry
to enable easy access to a lazily initializedLowerModule
instance. Its header is public for all CIR libraries, but it does not rely on other internal headers in the TargetLowering library.Before this PR, the initialization process was as follows:
CallConvLoweringPass
, a newLowerModule
is created for each function.LowerToLLVMPass
, a newLowerModule
is created exactly once.With this PR, each user initializes
LowerModule
if it is not yet initialized. Thus, there are three possible initialization points:CIRGenConsumer::HandleTranslationUnit
, if called from the Clang driver.CallConvLoweringPass::runOnOperation
, ifCallConvLoweringPass
is enabled bycir-opt
.LowerToLLVMPass
, forcir-opt
andcir-translate
.i.e. in any case,
LowerModule
is created on demand only once. If we usescir-opt
without CallConvLowering pass, the registry will keep uninitialized till the end.