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

optimizer: SROA mutable(immutable(...)) case correctly #43239

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented Nov 26, 2021

Our SROA should be able to handle mutable(immutable(...)) case even
without "proper" alias analysis, since we eliminate immutable first
and then process mutable within our iterative approach.

It turns out it hasn't worked because after the immutable handling,
we keep the reference count before DCE, but don't update dead reference
count. This commit fixes it and now we should be able to eliminate more
allocations, e.g.:

julia> simple_sroa(s) = broadcast(identity, Ref(s))
simple_sroa (generic function with 1 method)

julia> s = "julia";

julia> simple_sroa(s); # compile

julia> @allocated(simple_sroa(s))
0

@nanosoldier runbenchmarks("broadcast" || "sparse" || "array" || "union", vs=":master")

(... and now I lose yet another motivative example for EscapeAnalysis.jl-based SROA :P)

@aviatesk aviatesk added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Nov 26, 2021
@nanosoldier
Copy link
Collaborator

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

@chriselrod
Copy link
Contributor

It turns out it hasn't worked because after the immutable handling,
we keep the reference count before DCE, but don't update dead reference
count. This commit fixes it and now we should be able to eliminate more
allocations, e.g.:

Was that a regression?

julia> simple_sroa(s) = broadcast(identity, Ref(s))
simple_sroa (generic function with 1 method)

julia> simple_sroa("compile"); @allocated(simple_sroa("julia"))
0

julia> versioninfo()
Julia Version 1.6.4-pre.2
Commit 1a92e56a4d* (2021-10-09 19:47 UTC)

@aviatesk
Copy link
Sponsor Member Author

Ah, it seems like the entire chunk of @allocated(simple_sroa("julia)) is fully compiled (and so it's not a very good example):

julia> code_typed() do
           simple_sroa("julia")
       end
1-element Vector{Any}:
 CodeInfo(
1 ─     goto #3 if not true
2nothing::Nothing
3 ┄     goto #4
4 ─     goto #5
5 ─     goto #6
6 ─     goto #7
7 ─     goto #8
8return "julia"
) => String

Allocation happens if we use the compiled code of simple_sroa:

julia> s = "julia"
"julia"

julia> @allocated(simple_sroa(s))
16

but with this PR, no allocation should happen.

Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

Looks sensible to me though, of course this entire bit of code is a bit messy and we may want better abstractions in the future.

Base automatically changed from avi/sroarefac to master November 29, 2021 02:09
Our SROA should be able to handle `mutable(immutable(...))` case even
without "proper" alias analysis, since we eliminate `immutable` first
and then process `mutable` within our iterative approach.

It turns out it hasn't worked at all because after the immutable
handling
we keep the reference count _before_ DCE, but didn't update dead
reference
count. This commit fixes it and now we should be able to eliminate more
allocations, e.g.:
```julia
julia> simple_sroa(s) = broadcast(identity, Ref(s))
simple_sroa (generic function with 1 method)

julia> simple_sroa("compile"); @allocated(simple_sroa("julia"))
0
```
@aviatesk aviatesk merged commit 1a1f3d7 into master Nov 29, 2021
@aviatesk aviatesk deleted the avi/fixsroa branch November 29, 2021 02:21
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…3239)

Our SROA should be able to handle `mutable(immutable(...))` case even
without "proper" alias analysis, since we eliminate `immutable` first
and then process `mutable` within our iterative approach.

It turns out it didn't work because after the immutable handling
we keep the reference count _before_ DCE, but didn't update dead 
reference count. This commit fixes it and now we should be able to 
eliminate more allocations, e.g.:
```julia
julia> simple_sroa(s) = broadcast(identity, Ref(s))
simple_sroa (generic function with 1 method)

julia> simple_sroa("compile"); @allocated(simple_sroa("julia"))
0
```
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…3239)

Our SROA should be able to handle `mutable(immutable(...))` case even
without "proper" alias analysis, since we eliminate `immutable` first
and then process `mutable` within our iterative approach.

It turns out it didn't work because after the immutable handling
we keep the reference count _before_ DCE, but didn't update dead 
reference count. This commit fixes it and now we should be able to 
eliminate more allocations, e.g.:
```julia
julia> simple_sroa(s) = broadcast(identity, Ref(s))
simple_sroa (generic function with 1 method)

julia> simple_sroa("compile"); @allocated(simple_sroa("julia"))
0
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants