-
Notifications
You must be signed in to change notification settings - Fork 81
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
Transition GPUArrays to KernelAbstractions #451
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on this PR (finally). Can we also just remove indexing.jl
and synchronization.jl
for KA's utilities?
|
||
abstract type AbstractGPUBackend end | ||
|
||
abstract type AbstractKernelContext end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intention to remove AbstractKernelContext
completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think so. It shouldn't be needed anymore.
error("Not implemented") # COV_EXCL_LINE | ||
# TODO: | ||
# - Rename KA device to backend | ||
# - Who owns `AbstractGPUBackend`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, AbstractGPUBackend
is only used in the JLArrays lib, but can't we work towards deprecating that lib with KA's CPU capabilities?
If so, why do we need AbstractGPUBackend
? Can't we just use KA's Backend
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a JLArrays implementation using KA first.
idx = linear_index(ctx) | ||
idx > length(a) && return | ||
@kernel rand!(a, randstate) | ||
idx = @index(Global, Linear) | ||
@inbounds a[idx] = gpu_rand(T, ctx, randstates) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only way people will be using gpu_rand()
? Or will they be calling the functions on lines {33, 40, 58...}?
If so, we can just pass idx
as threadid
into the above functions, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only supported API are the Random stdlib functions; calling gpu_rand
explicitly is not supported, so feel free to refactor.
Ideally, yes, it would be nice if we can remove all of the |
Great! I'm working on this for like an hour or two a day, so expect a PR next week? #517 is kinda in the right direction, but I have another branch that is rebased up to master so I can test ROC / CuArrays at least. |
Can this be closed? |
Depends on JuliaGPU/KernelAbstractions.jl#317
Can happen last as long as we decide where backend lives.