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

isbits Fields and FieldSets #163

Closed
ali-ramadhan opened this issue Apr 3, 2019 · 4 comments
Closed

isbits Fields and FieldSets #163

ali-ramadhan opened this issue Apr 3, 2019 · 4 comments
Labels
abstractions 🎨 Whatever that means cleanup 🧹 Paying off technical debt GPU 👾 Where Oceananigans gets its powers from
Milestone

Comments

@ali-ramadhan
Copy link
Member

Would be good to adapt our Field structs to be isbits for CUDA kernels.

Same with FieldSet structs, although FieldVectors might be a good choice here. We would have to adapt FieldVector ourselves though.

See this PR for an example on how to adapt: JuliaArrays/OffsetArrays.jl#57

@ali-ramadhan ali-ramadhan added this to the v1.0 milestone Apr 3, 2019
@ali-ramadhan ali-ramadhan added abstractions 🎨 Whatever that means cleanup 🧹 Paying off technical debt GPU 👾 Where Oceananigans gets its powers from labels Apr 3, 2019
@glwagner
Copy link
Member

glwagner commented Apr 4, 2019

Related to this, I wanted to point something out:

julia> struct Test
       a::Array{Float64, 1}
       end

julia> import Base: getindex

julia> getindex(t::Test, inds...) = getindex(t.a, inds...)
getindex (generic function with 196 methods)

julia> t = Test(rand(4))
Test([0.502462, 0.632246, 0.585965, 0.845577])

julia> @code_lowered t.a[2]
CodeInfo(
731 1%1 = (Base.arrayref)($(Expr(:boundscheck)), A, i1)                                                                                                  │
    └──      return %1                                                                                                                                      │
)

julia> @code_lowered t[2]
CodeInfo(
1 1%1 = (Base.getproperty)(t, :a)                                                                                                                        │
  │   %2 = (Core.tuple)(%1)                                                                                                                                 │
  │   %3 = (Core._apply)(Main.getindex, %2, inds)                                                                                                           │
  └──      return %3                                                                                                                                        │
)

julia> @code_lowered @inbounds t.a[2]
CodeInfo(
538 1nothing                                                                                                                                        │
    │   %2 = (Base.Expr)(:inbounds, true)                                                                                                                   │
    │   %3 = (Base.esc)(blk)                                                                                                                                │
    │   %4 = (Base.Expr)(:(=), :val, %3)                                                                                                                    │
    │   %5 = (Base.Expr)(:local, %4)                                                                                                                        │
    │   %6 = (Base.Expr)(:inbounds, :pop)                                                                                                                   │
    │   %7 = (Base.Expr)(:block, %2, %5, %6, :val)                                                                                                          │
    └──      return %7                                                                                                                                      │
)

julia> @code_lowered @inbounds t[2]
\CodeInfo(
538 1nothing                                                                                                                                        │
    │   %2 = (Base.Expr)(:inbounds, true)                                                                                                                   │
    │   %3 = (Base.esc)(blk)                                                                                                                                │
    │   %4 = (Base.Expr)(:(=), :val, %3)                                                                                                                    │
    │   %5 = (Base.Expr)(:local, %4)                                                                                                                        │
    │   %6 = (Base.Expr)(:inbounds, :pop)                                                                                                                   │
    │   %7 = (Base.Expr)(:block, %2, %5, %6, :val)                                                                                                          │
    └──      return %7                                                                                                                                      │
)

Note that the lowered code for an inbounds getindex call is exactly the same for Test as it is for it's field, Test.a. There is a difference when the array access is not inbounds. So we probably shouldn't see slow down for using getindex on Field directly if its inbounds.

@glwagner
Copy link
Member

glwagner commented Apr 4, 2019

Another thing I noticed is that @inbounds is used in places where it can't really be guaranteed, like inside difference operator functions. I couldn't figure out how to test this, but I wonder if @inbounds propagates to all of the functions that are called inside its scope (is that the right language)?

Specifically I'm referring to patterns like the RHS calculation for u, which is @inbounds, but subsequently calls operators like this that are also have @inbounds.

In other words, we only need one @inbounds statement; we don't need multiple nested @inbounds statements. @vchuravy can you confirm/deny this?

A separate issue with @inbounds in places where it can't be guaranteed is that if users write functions / analysis tools using those operators, they might screw up and give out-of-bounds indices to functions that have guaranteed @inbounds (and in consequence get mysterious segfaults?) It seems the natural place for @inbounds is in the scope of a loop.

@glwagner
Copy link
Member

I propose we abandon this goal. We probably want to use shared memory in the future, which means that all of our functions need to operate on arrays --- and that renders the purpose of isbits fields moot.

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Aug 28, 2019

Hmmm, yeah I'll close it for now as we haven't discussed it in forever. The way the Field abstraction seems to have evolved is that we expose Fields to users, but behind the scenes we have to get a little messy and use either the OffsetArray or the underlying Array/CuArray as needed.

Which is fine, as long as the user interface is clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abstractions 🎨 Whatever that means cleanup 🧹 Paying off technical debt GPU 👾 Where Oceananigans gets its powers from
Projects
None yet
Development

No branches or pull requests

2 participants