Skip to content

Introduce temporary if necessary to inline _apply #21069

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

Merged
merged 4 commits into from
Mar 18, 2017
Merged

Conversation

martinholters
Copy link
Member

With

julia> function foo(x,y)
           println("x=$x, y=$y")
           return x, y
       end
foo (generic function with 1 method)

julia> bar(x,y) = +(foo(x,y)...)
bar (generic function with 1 method)

master:

julia> @code_typed bar(1, 2)
CodeInfo(:(begin 
        return (Core._apply)(Main.+, $(Expr(:invoke, MethodInstance for foo(::Int64, ::Int64), :(Main.foo), :(x), :(y))))::Int64
    end))=>Int64

this PR:

julia> @code_typed bar(1, 2)
CodeInfo(:(begin 
        #temp# = $(Expr(:invoke, MethodInstance for foo(::Int64, ::Int64), :(Main.foo), :(x), :(y)))
        return (Base.add_int)((Core.getfield)(#temp#, 1)::Int64, (Core.getfield)(#temp#, 2)::Int64)::Int64
    end))=>Int64

Fixes #21065 and makes #21067 (almost?) obsolete. (The "almost" being a matter of inlining promote, promote_type, and convert, where #21067 might still be advantageous.)

martinholters referenced this pull request Mar 17, 2017
@martinholters
Copy link
Member Author

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

tmpv = aarg
else
# apply(f,t::(x,y)) => tmp=t; f(tmp[1],tmp[2])
tmpv = add_slot!(sv.src, t, false)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should use newvar! and make an SSAValue instead. (That function is badly named; should be e.g. new_ssavalue.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated accordingly.

@martinholters
Copy link
Member Author

I wasn't sure whether the effect_free test is still necessary. Is there any reason the SSAValue should not be introduced unconditionally?

Also, what's a good way to test this PR? Inspect code_typed(bar, Tuple{Int,Int})[1].first.code and verify there is no :(Core._apply) in there? Do we have similar tests somewhere?

@martinholters
Copy link
Member Author

Added a test, but it does look a bit messy...

@JeffBezanson
Copy link
Member

Looks fine to me. There is a test kind of similar to this in test/inline.jl, but we usually just test performance and behavior.

Keeping the effect_free test is fine. We could introduce the ssavalue unconditionally and let it get removed by remove_redundant_temp_vars!, but it doesn't really matter.

@nanosoldier
Copy link
Collaborator

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

@JeffBezanson JeffBezanson merged commit e097911 into master Mar 18, 2017
@tkelman
Copy link
Contributor

tkelman commented Mar 18, 2017

would have been better to squash merge this

@tkelman tkelman deleted the mh/fix_21065 branch March 18, 2017 02:59
martinholters added a commit that referenced this pull request Mar 18, 2017
martinholters added a commit that referenced this pull request Mar 20, 2017
Backport of #21069.

(cherry picked from commit f65f273)
martinholters added a commit that referenced this pull request Mar 20, 2017
Backport of #21069.

(cherry picked from commit 038390b)
martinholters added a commit that referenced this pull request Mar 20, 2017
Backport of #21069.

(cherry picked from commit 1594692)
tkelman added a commit that referenced this pull request May 3, 2017
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.

5 participants