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

Suggestion - Disable AbstractArray indexing fallback by default #178

Closed
ViralBShah opened this issue Sep 1, 2017 · 6 comments · Fixed by JuliaGPU/GPUArrays.jl#359
Closed
Labels
cuda array Stuff about CuArray. speculative Not sure about this one yet.

Comments

@ViralBShah
Copy link
Contributor

If you mix CuArrays with Base Arrays, you get the slow version of things. I think this should best be disabled by default and give a nice error. Users should be able to opt in to enable it.

@MikeInnes
Copy link
Contributor

This is not quite as easy as it sounds, unfortunately. We can't tell in general that scalar indexing is bad (nor that vector indexing is good), so there are a few valid use cases that this would break by default. On top of that, all of the Base show methods rely on scalar indexing; breaking show methods leads to an overall worse experience, I think.

That said it would obviously be good to avoid confusion / slowness in some obvious cases. We can easily disable mixed broadcasting, and intercept the fallbacks for unimplemented linalg etc (although most of that stuff should be wrapped by now).

@maleadt maleadt transferred this issue from JuliaGPU/CuArrays.jl May 27, 2020
@maleadt maleadt added cuda array Stuff about CuArray. speculative Not sure about this one yet. labels May 27, 2020
@dpsanders
Copy link

I think it would make sense to change the default to

CUDA.allowscalar(false)

but still allow the user to specify

CUDA.allowscalar(true),

i.e. the reverse of the current situation, since my guess is that the majority of codes out there start with CUDA.allowscalar(false).

@ViralBShah
Copy link
Contributor Author

Yes, that was basically what I was suggesting.

@maleadt
Copy link
Member

maleadt commented May 29, 2020

A situation where this might degrade the experience:

julia> using CUDA
julia> sin.(CUDA.rand(Float32, 2))
2-element CuArray{Float32,1,Nothing}:
 0.08872609
 0.03352104
julia> sin.(CUDA.rand(Int, 2))
┌ Warning: Performing scalar operations on GPU arrays: This is very slow, consider disallowing these operations with `allowscalar(false)`
└ @ GPUArrays ~/.julia/packages/GPUArrays/HGtNV/src/host/indexing.jl:43
ERROR: MethodError: no method matching sin(::Int64)
You may have intended to import Base.sin

A failing broadcast falls back to scalar iteration -- disabling so would result in never seeing the MethodError.

We could make the warning scarier though.

@zhangcx93
Copy link

Can we have a global settings for disabling scaler index? like a environment variable?
I found in Pluto, using CUDA.allowscalar(false) doesn't have any effect. So I can put the ENV["CUDAallowscalar"] = false in startup.jl, and it'll do the job. And for those who want to make it default, it's a easy solution.

@zhangcx93
Copy link

Oh I found the ENV["JULIA_GPU_ALLOWSCALAR"] in source code... how about a doc for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda array Stuff about CuArray. speculative Not sure about this one yet.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants