Making @atomic robust #467
Conversation
|
I would like to know what other conditions I need to check for. Hence I have added |
|
I'd just flip the condition, requiring @cuda ((a)->(CUDAnative.@atomic a[2] = i; nothing))(CuArray{Int}(undef, 1))
ERROR: LoadError: type Symbol has no field headAlso add a test. Of course, as I mentioned on Slack it would be nice if this would just work, but that's more work. Improving the error message is a good start. |
|
Where and how do I add a test? |
|
Have a look at the |
|
I have added a section for |
src/device/cuda/atomics.jl
Outdated
| @@ -319,6 +319,9 @@ macro atomic(ex) | |||
| if ex.head == :(=) | |||
| ref = ex.args[1] | |||
| rhs = ex.args[2] | |||
| if typeof(rhs) <: Number || typeof(rhs) <: String || typeof(rhs) <: Symbol | |||
There was a problem hiding this comment.
That's not what I meant, instead of checking against a blacklist of non-Expr types, it's easier to (whitelist) check for what's allowed: Expr.
test/device/cuda.jl
Outdated
| @testset "exceptions" begin | ||
| @testset for T in [Int32, Int64, UInt32, UInt64] | ||
| a = CuArray([typemax(T)]) | ||
| function kernal(T, a) | ||
| @atomic a[1] = 1 | ||
| @atomic a[1] = "1" | ||
| @atomic a[1] = i | ||
| return | ||
| end | ||
| @cuda threads=32 kernal(T, a) | ||
| @test Array(a)[1] == 1 | ||
| end |
There was a problem hiding this comment.
Did you run these tests? They are intended to fail, so you can't just run them (the test would fail). Instead, you need to test_throws. Also, you can't put multiple failing atomic expressions in a single test because it'll bail out at the first error.
I'll push a fix, since it's somewhat finicky to use test_throws with macros.
|
bors try |
tryBuild succeeded |
|
Let's merge this. Thanks for the PR! |
This PR aims to handle various errors encountered by
@atomicand report them nicelyFixes #453