-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Increase codegen thread safety #44454
Increase codegen thread safety #44454
Conversation
PM.run(M); | ||
{ | ||
//Lock around our pass manager | ||
std::lock_guard<std::mutex> lock(this->mutex); |
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.
priority inversion? Can you add these locks to doc/src/devdocs/locks.md
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.
Would this be considered a level 1 lock since it never holds another lock within its scope?
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.
Yeah, if it does not need any others, which seems might be the case? That means our LLVM passes must not use any Julia state (global lookups, etc), but that seems probably true anyways.
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.
This probably doesn't affect us right now, since all of our work is done under the codegen lock anyways, but I wonder if the targetmachine used in llvm-cpufeatures will bite us here later then.
d1b8f71
to
7788e7d
Compare
7788e7d
to
47d3d26
Compare
|
||
static const auto jl_new_opaque_closure_jlcall_func = new JuliaFunction{XSTR(jl_new_opaque_closure_jlcall), get_func_sig, get_func_attrs}; | ||
|
||
static int globalUnique = 0; | ||
static std::atomic<int> globalUniqueGeneratedNames{0}; |
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.
Any particular reason you give these two different variables the same name now? Shouldn't matter for too much longer, as we are hopefully close to eliminating this counter, but just wondering
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.
They originally had the same name, and they looked like they performed the same function, so I was just preserving the similarity. I can revert that back to globalUnique.
47d3d26
to
c72d818
Compare
e46c9a3
to
ee02c05
Compare
1be7d85
to
a91f25f
Compare
a91f25f
to
583d06e
Compare
Removes a few global variables, marks some other ones as const where possible, and makes globalUnique an atomic integer.
Depends on #43770 for removing other LLVM globals.