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

Add @inline for anonymous functions #34953

Merged
merged 8 commits into from
Mar 15, 2020
Merged

Conversation

ssikdar1
Copy link
Contributor

@ssikdar1 ssikdar1 commented Mar 1, 2020

Closes #34939

added a check for ex.head === :-> in findmeta so that @inline will work anonymous functions.

Testing


julia> @inline x -> x^2 + 2x - 1
#1 (generic function with 1 method)

julia> @inline x -> x
#3 (generic function with 1 method)
  • Add a few tests.
    Added a test in test/compiler/inline.jl

  • Add a NEWS item.

@fredrikekre
Copy link
Member

Should we also look for

@inline f = x -> x

expressions?

@fredrikekre fredrikekre added the needs news A NEWS entry is required for this change label Mar 2, 2020
@KristofferC
Copy link
Member

KristofferC commented Mar 2, 2020

IMO, that is not necessary, this doesn't work for example:

@inline g = function f(x) x end

Sure, you wouldn't write that but it still is easiest if the @inline is only applied to the function definition expression.

@dkarrasch dkarrasch removed the needs news A NEWS entry is required for this change label Mar 2, 2020
function test_outer2(x)
@inline x -> x^2 + 2x - 1
end
test_inlined_symbols(test_outer2, Tuple{Int64})
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't test that the inline declaration has been applied to the inner function; it tests that the inlining pass produced valid output for test_outer2.

Copy link
Contributor Author

@ssikdar1 ssikdar1 Mar 2, 2020

Choose a reason for hiding this comment

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

@JeffBezanson
makes sense! How do I test if the inline declaration has been applied to the function?
Do I have to look at the Expr produced when using the inline macro?

julia> Base.parse_input_line("x -> x^2 + 2x - 1")
:($(Expr(:toplevel, :(#= none:1 =#), :((x->begin
          #= none:1 =#
          (x ^ 2 + 2x) - 1
      end)))))

julia> Base.parse_input_line("@inline x -> x^2 + 2x - 1")
:($(Expr(:toplevel, :(#= none:1 =#), :(#= none:1 =# @inline((x->begin
              #= none:1 =#
              (x ^ 2 + 2x) - 1
          end))))))

Something like this?

Copy link
Member

Choose a reason for hiding this comment

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

This is an easy way to test whether the inlining flag is set:

julia> ccall(:jl_ast_flag_inlineable, Bool, (Any,), first(methods(x->x)).source)
false

Copy link
Contributor Author

@ssikdar1 ssikdar1 Mar 2, 2020

Choose a reason for hiding this comment

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

@JeffBezanson
thanks! I added some tests to check :jl_ast_flag_inlineable for both anonymous and normal functions in b8ddf2a

Copy link
Member

Choose a reason for hiding this comment

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

This test can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 57ecb07

NEWS.md Outdated Show resolved Hide resolved

@testset "check jl_ast_flag_inlineable for inline macro" begin
ret = ccall(:jl_ast_flag_inlineable, Bool, (Any,), first(methods(@inline x -> x)).source)
@test ret == true
Copy link
Member

Choose a reason for hiding this comment

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

The ret variable is redundant here; can just do @test ccall(... etc. It is also more idiomatic to use @test x and @test !x instead of @test x == true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 57ecb07

@JeffBezanson
Copy link
Member

Thanks for your patience in working through the review process @ssikdar1 ! LGTM now.

@KristofferC KristofferC merged commit 5808aea into JuliaLang:master Mar 15, 2020
@ssikdar1 ssikdar1 deleted the inline branch March 15, 2020 13:42
ravibitsgoa pushed a commit to ravibitsgoa/julia that referenced this pull request Apr 9, 2020
* Add @inline for anonymous functions

Co-authored-by: Rafael Fourquet <[email protected]>
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
* Add @inline for anonymous functions

Co-authored-by: Rafael Fourquet <[email protected]>
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.

Feature request: @inline for anonymous functions
7 participants