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

supports @inline/@noinline annotations within a function body #41312

Merged
merged 4 commits into from
Jun 23, 2021

Conversation

aviatesk
Copy link
Member

Separated from #40754 in order to make the review/discussion more easier and clearer.

The primary motivation for this change is to annotate
@inline/@noinline to anonymous functions created from do block:

f() do
    @inline # makes this anonymous function to be inlined
    ... # function body
end

We can extend the grammar so that we have special "declaration-macro"
supports for do-block functions like:

f() @inline do # makes this anonymous function to be inlined
    ... # function body
end

but I'm not sure which one is better.

Following the earlier discussion,
this commit implements the easiest solution.


/cc @dghosef , @tkf

Separated from #40754 for the sake of easier review.

The primary motivation for this change is to annotate
`@inline`/`@noinline` to anonymous functions created from `do` block:
```julia
f() do
    @inline # makes this anonymous function to be inlined
    ... # function body
end
```

We can extend the grammar so that we have special "declaration-macro"
supports for `do`-block functions like:
```julia
f() @inline do # makes this anonymous function to be inlined
    ... # function body
end
```
but I'm not sure which one is better.

Following [the earlier discussion](#40754 (comment)),
this commit implements the easiest solution.

Co-authored-by: Joseph Tan <[email protected]>
!!! note
If the function is trivial (for example returning a constant) it might get inlined anyway.
"""
macro noinline(ex)
esc(isa(ex, Expr) ? pushmeta!(ex, :noinline) : ex)
end
macro noinline() Expr(:meta, :noinline) end
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we can delete Core.@_noinline_meta() also?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think we can unify them. Will do that in a follow up PR though.

@JeffBezanson
Copy link
Member

f() @inline do

❌ Do not want! 😄

base/expr.jl Outdated Show resolved Hide resolved
base/expr.jl Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@aviatesk aviatesk merged commit 37c0b06 into master Jun 23, 2021
@aviatesk aviatesk deleted the avi/doinline branch June 23, 2021 00:30
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
…liaLang#41312)

* supports `@inline`/`@noinline` annotations within a function body

Separated from JuliaLang#40754 for the sake of easier review.

The primary motivation for this change is to annotate
`@inline`/`@noinline` to anonymous functions created from `do` block:
```julia
f() do
    @inline # makes this anonymous function to be inlined
    ... # function body
end
```

We can extend the grammar so that we have special "declaration-macro"
supports for `do`-block functions like:
```julia
f() @inline do # makes this anonymous function to be inlined
    ... # function body
end
```
but I'm not sure which one is better.

Following [the earlier discussion](JuliaLang#40754 (comment)),
this commit implements the easiest solution.

Co-authored-by: Joseph Tan <[email protected]>

* Update base/expr.jl

* Update base/expr.jl

* Update NEWS.md

Co-authored-by: Joseph Tan <[email protected]>
@timholy
Copy link
Member

timholy commented Sep 4, 2021

I've created the shiny new "Add to Compat" label. Keep in mind that's not intended to be a requirement for the PR author before merging, and it can be done by anyone, anytime. The intent is that by tagging PRs with this label, we may have a better sense for likely trouble when we start running release-validation tests, and it may also encourage dissemination of excellent new features across the package ecosystem.

While this looks eminently backportable via Compat.jl, the intent is that this can be used even when the only possible version that could be added to Compat.jl is a "stub" implementation that simply avoids an error, allowing the new construct to be used in packages even for Julia versions where it won't actually have any effect.

Motivated by #42087.

@aviatesk
Copy link
Member Author

aviatesk commented Sep 4, 2021

Sounds great, thanks.

Do you think we want to tag #41328 with the label also ?
Probably we may want to add the stub implementations of the callsite annotations to Compat.jl as well so that packages can keep being compatible with older versions, but they're just new features and won't lead to release-validation tests.

@timholy
Copy link
Member

timholy commented Sep 4, 2021

Added to #41328 and #41931.

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.

4 participants