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

Avoid boundchecking iteration #15291

Closed
wants to merge 1 commit into from
Closed

Avoid boundchecking iteration #15291

wants to merge 1 commit into from

Conversation

carnaval
Copy link
Contributor

Add an unsafe_next method which contract is that the iteration protocol was followed correctly. It has a generic fallback to next.
Lowering uses this instead of next. Wrapper iterators can also forward it.
This is demonstrated for Enumerate in this commit.

With this we can proudly say we compile a dead loop to nothing, yay...

@carnaval
Copy link
Contributor Author

This will allow us to avoid sprinkinling @inbounds at least for the trivial cases. At some point we'll probably need a redundant @inbounds checker to be able to remove the useless ones.

@@ -232,6 +232,11 @@ collect(itr) = collect(eltype(itr), itr)
## Iteration ##
start(A::Array) = 1
next(a::Array,i) = (a[i],i+1)
function unsafe_next(a::Array,i)
@inbounds begin
return next(a,i)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is still the case, but @inbounds return was difficult for inference to reason about at one point in recent history. You're probably more aware of the current design than I am, but just FYI: #13461.

@vtjnash
Copy link
Member

vtjnash commented Feb 29, 2016

would it be possible to use the new @boundscheck mechanism?

@carnaval
Copy link
Contributor Author

I think boundscheck is too coarse since if lowering insert a @inbounds next() and someone has a custom iterator that happens to index into an array in next() then we would be incorrectly removing the bound check.

unsafe_next has the advantage that the contract is only about the iterator passed as an argument, which is the only thing for-loop lowering guarantees

Add an unsafe_next method which contract is that the iteration protocol
was followed correctly.
@JeffBezanson
Copy link
Member

I wonder if this plus #15259 will fix the performance gap in #15123 (which implements map using collect and zip).

@carnaval
Copy link
Contributor Author

carnaval commented Mar 2, 2016

I'm open to suggestions for design, maybe unsafe_next is too painful and it apparently could use the new boundscheck thing according to Jameson. In any case it is kind of silly that we boundcheck one of the only guaranteed safe indexing construct in the language :-)

@blakejohnson
Copy link
Contributor

I guess an alternative approach would be to directly inject (inbounds true) ... (inbounds false) around the call to next when lowering the expression.

@blakejohnson
Copy link
Contributor

Or, write next as something like:

next(a::Array, i) = (@boundscheck checkbounds(a, i); @inbounds r=a[i]; (r, i+1))

Though, to ellide the bounds check you still need to decorate the next call site with @inbounds.

@vtjnash vtjnash closed this Jul 14, 2016
@vtjnash
Copy link
Member

vtjnash commented Jul 14, 2016

Closing, as I believe we do this now via @boundscheck

@vtjnash vtjnash deleted the ob/ibit branch July 14, 2016 17:48
@yuyichao
Copy link
Contributor

Do we? I think this is about removing bounds check automatically for for ele in a::Array without @inbounds The @boundscheck change was actually before this PR.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2017

Any chance of resurrecting this PR in some form?

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.

7 participants