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.

Sign up for GitHub

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

Jump to bottom

Force specialization on operator arguments for mapfoldl_impl #33917

Merged
merged 1 commit into from
Nov 22, 2019
Merged

Force specialization on operator arguments for mapfoldl_impl #33917

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions base/reduce.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ mul_prod(x::Real, y::Real)::Real = x * y

## foldl && mapfoldl

function mapfoldl_impl(f, op, nt::NamedTuple{(:init,)}, itr, i...)
function mapfoldl_impl(f::F, op::OP, nt::NamedTuple{(:init,)}, itr, i...) where {F,OP}
Copy link
Member

Choose a reason for hiding this comment

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

Migth be worth a comment, as the where {F,OP} is here purely for this unusual reason of forcing specialization, which may not be evident to the next person touching these lines (who might as well, with good intentions, revert this PR unknowingly :) )

Copy link
Member Author

Choose a reason for hiding this comment

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

We might want to do that broadly across Julia's code base. Grep string in #32761 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Ah right! Would it stretch too much the meaning of @specialize to extend this macro to formalize this technique? E.g. @specialize(f), @specialize(op) in this case, even if no @nospecialize was active?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be great if @specialize did this. I didn't even realize we had it! There's a discourse post from a while back making this exact suggestion.

init = nt.init
# Unroll the while loop once; if init is known, the call to op may
# be evaluated at compile time
Expand All @@ -51,7 +51,7 @@ function mapfoldl_impl(f, op, nt::NamedTuple{(:init,)}, itr, i...)
return v
end

function mapfoldl_impl(f, op, nt::NamedTuple{()}, itr)
function mapfoldl_impl(f::F, op::OP, nt::NamedTuple{()}, itr) where {F,OP}
y = iterate(itr)
if y === nothing
return Base.mapreduce_empty_iter(f, op, itr, IteratorEltype(itr))
Expand Down