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

Views with reshaped range indices are linear and strided #34729

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Feb 11, 2020

This came up in a DiffEq task, wherein a matrix is stored as a subset of the parameters vector — and thus to be used, it must be reshaped/subsetted before doing a matmul. This patch gives such a view access to the blassy goodness. The exemplar case looks something like this:

p = rand(300)
M = view(p, reshape(2:241, 60, 4))
v = rand(4)
@benchmark $M*$v

Before, this took nearly 200ns. With this patch we hit BLAS and are down to 100ns. Interestingly, this introduces a bifurcation where it's possible for view to be linear but not strided... but fortunately we've been pretty strict on keeping those two attributes distinct even though they previously were 100% overlapping.

I made sure the SubArray tests passed with JULIA_TESTFULL and while I was there I testsetified the test/subarray.jl file (as a separate and independent commit).

cc: @ChrisRackauckas.

@mbauman mbauman added the performance Must go faster label Feb 11, 2020
ChrisRackauckas added a commit to SciML/DiffEqFlux.jl that referenced this pull request Feb 11, 2020
Should get merged after JuliaLang/julia#34729 with an `if` for the version number.
@mbauman mbauman force-pushed the mb/reshapedrangeviewindex branch 2 times, most recently from e88082b to 956acdd Compare February 14, 2020 21:56
@mbauman mbauman requested a review from timholy February 14, 2020 23:07
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

This looks great. TBH I only looked carefully at the first commit, I assume that has all the functional changes? Thanks also for the cleanup of the subarray tests, I noticed several nice improvements there.

@ChrisRackauckas
Copy link
Member

bump?

@ChrisRackauckas
Copy link
Member

Bump again? I'd like to see this in v1.5 if possible.

@ViralBShah
Copy link
Member

@mbauman Can we get this in? Just have 4 more days!

@mbauman mbauman merged commit cf59f6d into master Apr 30, 2020
@mbauman mbauman deleted the mb/reshapedrangeviewindex branch April 30, 2020 15:29
ChrisRackauckas added a commit to SciML/DiffEqFlux.jl that referenced this pull request May 10, 2020
Should get merged after JuliaLang/julia#34729 with an `if` for the version number.
Utilize view strided fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants