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

GPU issue indexing flat fields #4044

Open
jagoosw opened this issue Jan 14, 2025 · 7 comments
Open

GPU issue indexing flat fields #4044

jagoosw opened this issue Jan 14, 2025 · 7 comments
Labels
GPU 👾 Where Oceananigans gets its powers from

Comments

@jagoosw
Copy link
Collaborator

jagoosw commented Jan 14, 2025

When I try to index into a field which has nothing topology in a kernel, e.g.:

@kernel function add_one!(A)
    i, j = @index(Global, NTuple)
    A[i, j] += 1
end

then it works on CPU, but hits a dynamic function invocation on line 82 here:

@inline function Base.size(loc, topo, sz, indices=default_indices(Val(length(loc))))
D = length(loc)
# (it's type stable?)
return ntuple(Val(D)) do d
Base.@_inline_meta
length(instantiate(loc[d]), instantiate(topo[d]), sz[d], indices[d])
end
end

could we rewrite this like:

@inline function Base.size(loc::NTuple{N},...) where N

instead?

@jagoosw jagoosw added the GPU 👾 Where Oceananigans gets its powers from label Jan 14, 2025
@simone-silvestri
Copy link
Collaborator

The problem here is actually this line

Base.size(f::AbstractField) = size(f.grid, location(f))

that gets called here
@inline function Base.axes(f::Abstract3DField)

It is not a problem on CPUs because on CPUs we have a grid associated to a field. On GPUs we throw away the grid so this function errors. I can open a PR to fix this

@simone-silvestri
Copy link
Collaborator

simone-silvestri commented Feb 11, 2025

PS this does not happen if you index with 3 indices since axes does not need to be called since we have specific methods for 3d indexing of reduced fields

@glwagner
Copy link
Member

Before you fix this @simone-silvestri, the issue is that the indexing should be A[i, j, 1]

@simone-silvestri
Copy link
Collaborator

I see a lot of users try to access 2D fields with [i, j], which does make sense (this was supported in Oceananigans up to the change to axes. I think it should be included. Open to not include it though.

@glwagner
Copy link
Member

Hmm interesting. I think that support was an accident?

@glwagner
Copy link
Member

I guess we have to decide whether its important for people to explicitly declare the dead indices or not. Another consideration is whether this permits more general code (eg reduced code independent of direction). I haven't seen the latter though.

@glwagner
Copy link
Member

Somehow internally in Oceananigans we have never used a 1- or 2-index form, but @simone-silvestri you're saying this appears in user code? If you have examples that will help shape this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GPU 👾 Where Oceananigans gets its powers from
Projects
None yet
Development

No branches or pull requests

3 participants