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

adding attempt to force inbounds at the kernel level #429

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

leios
Copy link
Member

@leios leios commented Nov 2, 2023

This adds an additional flag at the @kernel level to force @inbounds across the entire kernel. It works like:

julia> @kernel inbounds=true function check_inline(a)
           a[5] = 5
       end
julia> a = [0,0];
julia> backend = get_backend(a);
julia> kernel! = check_inline(backend, 256);
julia> kernel!(a, ndrange = 10) # no error!

Note that this comes with all the caveats of using @inbounds and is certainly not good for default behaviour, but... Geez I'm tired of all the @inbounds

I'll rebase and add some tests soon if people think this could be useful.

src/KernelAbstractions.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (c1c6887) 83.84% compared to head (d65431e) 83.28%.

❗ Current head d65431e differs from pull request most recent head ddb25a5. Consider uploading reports for the commit ddb25a5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
- Coverage   83.84%   83.28%   -0.56%     
==========================================
  Files           9        9              
  Lines         625      634       +9     
==========================================
+ Hits          524      528       +4     
- Misses        101      106       +5     
Files Coverage Δ
src/KernelAbstractions.jl 83.42% <78.57%> (-0.01%) ⬇️
src/macros.jl 97.04% <40.00%> (-0.02%) ⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leios
Copy link
Member Author

leios commented Nov 2, 2023

Ok, I think this PR is done. I made it kinda on a whim and I'm not sure if people actually want it. I could use it for a few specific workflows and think it might be useful for debugging JuliaGPU/AMDGPU.jl#536

Maybe we can wait a few days to see if anyone else thinks it could be useful?

The way I see it, it's one of those things that makes the code more "kernel" like, but can also really hurt some people who are not careful.

@vchuravy
Copy link
Member

vchuravy commented Nov 2, 2023

It's non breaking so I am happy to include it.

test/runtests.jl Outdated
@test_throws BoundsError(Int64[],(1,)) my_bounded_kernel(CPU())(Int[], ndrange=1)

@kernel inbounds=true my_inbounds_kernel(a) = a[1]
@test nothing == my_inbounds_kernel(CPU())(Int[], ndrange=1)
Copy link
Member

Choose a reason for hiding this comment

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

I think CI runs with --check-bounds=true which overrides @inbounds.

@luraess
Copy link

luraess commented Nov 2, 2023

Ok, I think this PR is done. I made it kinda on a whim and I'm not sure if people actually want it. I could use it for a few specific workflows and think it might be useful for debugging JuliaGPU/AMDGPU.jl#536

Maybe we can wait a few days to see if anyone else thinks it could be useful?

The way I see it, it's one of those things that makes the code more "kernel" like, but can also really hurt some people who are not careful.

That's nice to have! And can definitively help while debugging and developing.

@lcw
Copy link
Collaborator

lcw commented Nov 2, 2023

Love it!

@leios
Copy link
Member Author

leios commented Nov 2, 2023

Ok, looks like some other folks might find this useful. Also, after looking at JuliaGPU/AMDGPU.jl#354, and JuliaLang/julia#48245, I think this is a fair PR to merge.

Also --check-bounds=yes is set by default for Pkg.test. Does anyone know how to check whether this environmental variable is on or off in Julia itself? Otherwise, I am not sure how to test that the macro is actually successful.

EDIT: Base.JLOptions().check_bounds

Co-authored-by: Valentin Churavy <vchuravy@users.noreply.github.com>
@leios
Copy link
Member Author

leios commented Nov 2, 2023

Ok, I think the tests pass (except nightly). So I think the PR is good to go.

@vchuravy vchuravy merged commit 0aabbcb into JuliaGPU:main Nov 2, 2023
25 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants