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

Teach @polly to simplify range-based loops on AST-level #17965

Conversation

7 participants
@MatthiasJReisinger
Copy link
Contributor

commented Aug 11, 2016

@polly now rewrites loops that iterate over Base.UnitRange or
Base.StepRange to use the newly introduced Polly.UnitRange or
Polly.StepRange instead. These loops will be lowered to LLVM IR
where the loop bounds are more obvious to Polly's underlying
analyses than when using the Base ranges.

However, the new ranges might actually result in code that's slightly
less efficient than for Base ranges. For example, in the following
loop I noted a slowdown of about 3 % between Base.UnitRange(1,100000)
and Polly.UnitRange(1,100000):

function f(r)
    s = 0.0
    for i in r
        s += sin(i)
    end
    return s
end

This is the reason why for now we use the new ranges only in code
parts that are touched by Polly and where possible slowdowns would
therefore be able to be compensated by Polly's optimizations.

The described additions are available in the new file base/polly.jl
where we also moved the @polly macro definition.

According discussions on this issue also took place in
https://groups.google.com/forum/#!topic/julia-dev/5io1KnEswqs and
https://groups.google.com/forum/#!topic/julia-dev/6LZaSnGCqno

@tobig @vtjnash @timholy

@@ -0,0 +1,156 @@
# This file is a part of Julia. License is MIT: http://julialang.org/license

This comment has been minimized.

Copy link
@tkelman

tkelman Aug 11, 2016

Contributor

you'll need to add this file to the list in test/choosetests.jl if you want it to run done

return esc(Base.pushmeta!(func, :polly))
end

# This range type only differs from `Base.UnitRange` in it's representation of

This comment has been minimized.

Copy link
@tkelman

tkelman Aug 11, 2016

Contributor

its

# and `i = Polly.StepRange(start,step,stop)`.
function canonicalize_ranges_in_loop_header!(loop_header)
if loop_header.head == :block
# If the loop header is a `:block` we are dealing with a loop of th

This comment has been minimized.

Copy link
@tkelman

tkelman Aug 11, 2016

Contributor

of the

canonicalize_range_in_assignment!(assignment)
end
else
# If the loop header is no `:block` we have just a simple `for i = ...`

This comment has been minimized.

Copy link
@tkelman

tkelman Aug 11, 2016

Contributor

is not ?

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/polly/range_canonicalization branch from e6b3d5c to eafb329 Aug 11, 2016

@MatthiasJReisinger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

Thank you for your comments @tkelman! I tried to address them in the updated PR.

@timholy

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

One threat to this approach is that, increasingly, code should probably be written in one of the idioms below.

To achieve good performance for both LinearFast and LinearSlow arrays:

for I in eachindex(A)   # I is often a CartesianIndex, although sometimes it will be a linear index
    s += A[I]
end

To be safe for general indices:

for j in indices(A,2), i in indices(A,1)   # indices(A,d) returns an AbstractUnitRange
    s += A[i,j]
end

You might need to check for these kinds of constructs.

Further reading:

@MatthiasJReisinger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2016

Thank you for pointing this out @timholy! I think these additions are kind of independent and could be addressed in a separate PR if that's fine from your side?

Best,
Matthias

@MatthiasJReisinger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2016

I took a closer look at indices and eachindex and as long as they return an AbstractUnitRange we would actually be fine because essentially UnitRanges are only problematic when both their start and stop values are statically unknown, or in scenarios like for i = 1:n, j = i:m where the inner range-bounds depend on the outer loop. Actually such cases could not happen with indices or eachindex since in the one-dimensional case they just seem to return 1:length(A).

@timholy

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

eachindex sometimes returns a CartesianRange. For example:

julia> a = reshape(1:9, 3, 3)
3×3 Base.ReshapedArray{Int64,2,UnitRange{Int64},Tuple{}}:
 1  4  7
 2  5  8
 3  6  9

julia> a = collect(reshape(1:9, 3, 3))
3×3 Array{Int64,2}:
 1  4  7
 2  5  8
 3  6  9

julia> s = view(a, 1:2, :)
2×3 SubArray{Int64,2,Array{Int64,2},Tuple{UnitRange{Int64},Colon},false}:
 1  4  7
 2  5  8

julia> eachindex(s)
CartesianRange{CartesianIndex{2}}(CartesianIndex{2}((1,1)),CartesianIndex{2}((2,3)))
@timholy

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

Also maybe of relevance:

julia> using OffsetArrays

julia> b = OffsetArray(a, -1:1, 0:2)
OffsetArrays.OffsetArray{Int64,2,Array{Int64,2}} with indices -1:1×0:2:
 1  4  7
 2  5  8
 3  6  9

julia> indices(b)
(-1:1,0:2)

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/polly/range_canonicalization branch from eafb329 to 9b1d82d Aug 22, 2016

@vtjnash

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Could this go into a Polly.jl package? That would also make it possible to use this on v0.5.x by decoupling development of it from the timeline of base releases.

@MatthiasJReisinger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

If we want to use Polly with Julia 0.5.x we would have to ensure that it stays compatible with LLVM's master branch since we currently need to build against the latest Polly and LLVM versions to get its latest enhancements as long as we are at an experimental stage. For the future I think it will definitely be a good idea to be able to maintain compatibility with multiple Julia versions and if that's eased by having a Polly.jl package I think that's the direction we should take. But for now it maybe won't bring us additional advantages for the above reasons. I'm not sure, what do you think?

@tobig

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

As this is still rather experimental work and I foresee further changes both on the LLVM, Polly and Julia side, I would suggest to keep this synchronized with Julia trunk and only move this into a package at the point where we can guarantee to provide a similar set of transformations for a larger set of core kernels.

@tkelman

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

We've already branched and I'd be against backporting something this experimental to 0.5. It's mostly the quantity and correctness of this code that you'd have more flexibility to keep updating outside of base.

@tobig

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

On Thu, Aug 25, 2016, at 01:26 AM, Tony Kelman wrote:

We've already branched and I'd be against backporting something this
experimental to 0.5. It's mostly the quantity and correctness of this
code that you'd have more flexibility to keep updating outside of base.

Right. I don't think we should spent time to support 0.5 with new
features, as this time is better spent to make 0.6 a round and impactful
release. So I don't think we want to back-port this work.

Regarding flexibility, I personally believe that good code reviews from
the core community are at the moment still very important as Matthias is
touching both code generation, Polly and these more high-level
transformations. Trading some flexibility to make sure that all these
three points are developed together and under the eyes and suggestions
of the community seems important.

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/polly/range_canonicalization branch 2 times, most recently from 972dcc1 to cbb5daf Sep 28, 2016

Teach @polly to simplify range-based loops on AST-level
`@polly` now rewrites loops that iterate over `Base.UnitRange` or
`Base.StepRange` to use the newly introduced `Polly.UnitRange` or
`Polly.StepRange` instead. These loops will be lowered to LLVM IR
where the loop bounds are more obvious to Polly's underlying
analyses than when using the `Base` ranges.

However, the new ranges might actually result in code that's slightly
less efficient than for `Base` ranges.  This is the reason why for
now we use the new ranges only in code parts that are touched by Polly
and where possible slowdowns would therefore be able to be compensated
by Polly's optimizations.

The described additions are available in the new file base/polly.jl
where we also moved the `@polly` macro definition.

According discussions on this issue also took place in
https://groups.google.com/forum/#!topic/julia-dev/5io1KnEswqs and
https://groups.google.com/forum/#!topic/julia-dev/6LZaSnGCqno

@MatthiasJReisinger MatthiasJReisinger force-pushed the MatthiasJReisinger:mjr/polly/range_canonicalization branch from cbb5daf to 2813ae5 Oct 28, 2016

@singam-sanjay

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

@MatthiasJReisinger Would you be interested in taking this forward ?

while !isempty(worklist)
expr = pop!(worklist)
if expr.head == :for
loop_header = expr.args[1]

This comment has been minimized.

Copy link
@singam-sanjay

singam-sanjay May 26, 2017

Contributor

Wow ! What's this feature of Julia that allows functions to be accessed like a parse tree ?

This comment has been minimized.

Copy link
@singam-sanjay

singam-sanjay May 26, 2017

Contributor

Thank You !

@MatthiasJReisinger

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2017

I'll not be able to work on this myself at the moment, but you can certainly pick it up if you are interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.