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

Handle setfield! in alloc_elim_pass #16969

Closed
wants to merge 1 commit into from
Closed

Handle setfield! in alloc_elim_pass #16969

wants to merge 1 commit into from

Conversation

carnaval
Copy link
Contributor

Probably does not come up very often but was added to allow for generic code with mutable accumulators such as

mut_zero(::Type{BigFloat}) = zero(BigFloat)
function mut_add!(x::BigFloat, y::BigFloat)
    ccall((:mpfr_add,:libmpfr), Int32, (Ptr{BigFloat}, Ptr{BigFloat}, Ptr{BigFloat}, Int32), &x, &x, &y, Base.MPFR.ROUNDING_MODE[end])
end

mut_zero(::Type{Float64}) = Ref(0.0)
mut_add!(x,y) = x[] += y
Base.convert(::Type{Float64}, r::Ref{Float64}) = r[]

then write

function sum{T}(vs::Vector{T})
    acc = mut_zero(T)
    for v in vs
        mut_add!(acc,v)
    end
    convert(T, acc)
end

and get efficient scalar code in the Float64 case

@andreasnoack

@yuyichao
Copy link
Contributor

👍 I didn't do this last time because I worry being more aggressive here might make the codegen bug worse.

@JeffBezanson
Copy link
Member

Cool!

While you're at it, it would be good to see if alloc_elim_pass can be sped up at all, since I've seen some profiles that indicate a fair amount of time is spent there.

I would also like to see comments illustrating the transformation done, i.e. showing example IR fragments and what they get converted to.

@carnaval
Copy link
Contributor Author

While you're at it, it would be good to see if alloc_elim_pass can be sped up at all, since I've seen some profiles that indicate a fair amount of time is spent there.

do you have an example benchmark where this is particularly evident that I can use as a basis ?

I would also like to see comments illustrating the transformation done, i.e. showing example IR fragments and what they get converted to.

can do

@JeffBezanson
Copy link
Member

It was one that @andreasnoack ran into:

a = rand(5,5);
LinAlg.generic_matmatmul!(a, 'N', 'N', a, a);

I don't think this pass is more than half the time, but I was hoping there might be low-hanging fruit there. In particular a lot of time is spent in occurs_outside_getfield.

@timholy
Copy link
Member

timholy commented Jun 16, 2016

With apologies for hijacking this for unrelated purposes, see #16128 (comment)

@nanosoldier runbenchmarks(ALL, vs=":release-0.4")

jrevels referenced this pull request Jun 16, 2016
try to simplify information flow around inference
@nanosoldier
Copy link
Collaborator

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

@vtjnash
Copy link
Member

vtjnash commented Jul 14, 2016

This looks pretty cool. The benchmarks are all over the place though: some got faster, some slower.

@jrevels
Copy link
Member

jrevels commented Jul 14, 2016

The benchmarks are all over the place though: some got faster, some slower.

Note that Tim ran the benchmarks here against the release-0.4 branch, not master (I think at the time he didn't know that benchmarks could be run from commits, so he hijacked this PR to do a comparison).

@andreasnoack
Copy link
Member

andreasnoack commented Jul 17, 2016

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

@yuyichao
Copy link
Contributor

Quote?

@jrevels
Copy link
Member

jrevels commented Jul 17, 2016

Nanosoldier doesn't accept jobs via comment edits (assuming that's what happened here). It used to, but I considered that a bug and fixed it (since it easily allowed jobs to accidentally get re-triggered, or triggered on comment deletion).

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

@andreasnoack
Copy link
Member

Thanks and I can see that the soldier actually changes color so I should have noticed.

@nanosoldier
Copy link
Collaborator

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

@andreasnoack
Copy link
Member

@Keno
Copy link
Member

Keno commented May 9, 2019

We have this now

@Keno Keno closed this May 9, 2019
@DilumAluthge DilumAluthge deleted the ob/aepsf branch March 25, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants