Skip to content
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

Emit safepoints at function entry #41616

Merged
merged 5 commits into from
Oct 25, 2022
Merged

Emit safepoints at function entry #41616

merged 5 commits into from
Oct 25, 2022

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Jul 16, 2021

Otherwise non-allocating functions like:

fib(x) = x <= 1 ? 1 : fib(x-1) + fib(x-2) will block GC from make progress.

This now inserts a safepoint at the beginning of every function.

Note:
Initially I wanted to insert these safepoints late and also into back-branches. That is currently not legal since at a safepoint we must gurantuee that all objects are rooted and legal. Legal means that the typetag is set correctly, and thus we shall not sink a store of a typetag past a safepoint, and rooting requires that stores of an object to a field shall not be sunk past a safepoint.
We can't currently gurantuee these properties after we already started optimizing the code.

@vtjnash had the idea that we could insert safepoints at the exit blocks of outer loops, that might be an idea for the future.

Besides inserting a safepoint here this has the additional overhead of requiring a lookup from a thread local variable + a load from the current ptls. In c76459c I hoisted the signal_page pointer load outside the fence.

@maleadt I made the emission a codegen feature so that we can turn it off for GPU code.

@vchuravy vchuravy added GC Garbage collector compiler:codegen Generation of LLVM IR and native code labels Jul 16, 2021
@vchuravy vchuravy requested review from vtjnash and yuyichao July 16, 2021 12:22
src/llvm-gc-safepoint.cpp Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member Author

Ah shoot, LateLowerGC of course doesn't see these as safepoints. So yeah we need to introduce julia.safepoint as an opaque function and lower the safepoint in FinalLower

@JeffBezanson
Copy link
Member

Very curious to see the performance impact of this. The function entry one we can maybe just go ahead with, but for loops I imagine we'll have to put state transitions around the loop instead of safepoints on every backedge.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe, for correctness, this must be inserted by codegen, before optimization passes run, as it adds a global side-effect (thread synchronization points) to the code. We can then write optimization passes that merge and delete them, or that convert them into safe-regions. But I am not sure it is legal to add them later.

@vchuravy
Copy link
Member Author

as it adds a global side-effect (thread synchronization points) to the code.

The safepoint has a pair off:

     B.CreateFence(AtomicOrdering::SequentiallyConsistent, SyncScope::SingleThread);

Which according to https://llvm.org/docs/LangRef.html#syncscope is:

If an atomic operation is marked syncscope("singlethread"), it only synchronizes with and only participates in the seq_cst total orderings of other operations running in the same thread (for example, in signal handlers).

So from a memory model point it should be legal to insert them late?

The function entry one we can maybe just go ahead with, but for loops I imagine we'll have to put state transitions around the loop instead of safepoints on every backedge.

Yeah that's my feeling as well. Hopefully have time to work on this next week.

@JeffBezanson JeffBezanson added the multithreading Base.Threads and related functionality label Jul 19, 2021
@vchuravy vchuravy changed the base branch from master to vc/emit_signal_fence July 22, 2021 19:26
@vchuravy
Copy link
Member Author

Jameson convinced me out-of-band that we can't late insert safepoints since LLVM might sink the tag store past a safepoint with out the memory fence. So this is doing the more minimal change of emitting a safepoint at the start of the function.

@vchuravy vchuravy changed the title Emit safepoints at function entry and loop backedges Emit safepoints at function entry Jul 23, 2021
@vchuravy vchuravy requested review from maleadt and vtjnash July 23, 2021 09:34
@vchuravy vchuravy marked this pull request as ready for review July 23, 2021 10:01
@vchuravy
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs=":master")

@vchuravy
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

Base automatically changed from vc/emit_signal_fence to master July 26, 2021 22:27
@JuliaLang JuliaLang deleted a comment from giveaway001 Jul 27, 2021
@vtjnash
Copy link
Member

vtjnash commented Sep 24, 2021

Need to teach it to remove this safepoint after codgen for functions which don't interact with the runtime anyways (hack for @threadcall), and rebase

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Oct 25, 2022
@giordano giordano merged commit 1a7a131 into master Oct 25, 2022
@giordano giordano deleted the vc/safepoints branch October 25, 2022 21:32
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Oct 25, 2022
KristofferC pushed a commit that referenced this pull request Dec 20, 2022
@KristofferC
Copy link
Member

I've reverted this on 1.9 due to #47826 to get it off the milestone.

kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Sep 27, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Sep 28, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Oct 4, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Oct 6, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Oct 19, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Oct 19, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Oct 20, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Oct 21, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Oct 23, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Nov 1, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Nov 2, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Nov 7, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Nov 10, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Nov 14, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Nov 15, 2023
* Emit safepoints at function entry

* Make safepoint emission on function entry a codegen feature

* Hoist signal page lookup outside fence

* Update src/cgutils.cpp

* Fix rebase
@vtjnash vtjnash mentioned this pull request Dec 21, 2023
Zentrik added a commit to Zentrik/julia that referenced this pull request Feb 2, 2024
Given JuliaLang#41616 merged I assume this workaround can be removed. I haven't tested it.
vtjnash pushed a commit that referenced this pull request Feb 12, 2024
Given #41616 merged, remove reference to it in doc example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code GC Garbage collector multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants