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

Remove println statements accompanying errors #172

Merged
merged 4 commits into from
Dec 13, 2020

Conversation

mcabbott
Copy link

This replaces println(ex); throw(str) with an error type, so that if macro expansion fails within a try-catch block, nothing is printed.

Closes mcabbott/Tullio.jl#26

Example of new behaviour:

julia> try                                                                   
       macroexpand(Main, quote                                               
       @avx for i = 𝒶𝓍i                                                      
           if i ∈ (axes)(var"𝛥≪rand(10)≫", 1)                                
               var"𝛥≪rand(10)≫"[i] = var"𝛥≪rand(10)≫"[i] + 𝛥ℛ[i]             
           else                                                              
               (zero)((eltype)(var"𝛥≪rand(10)≫"))                            
           end                                                               
       end                                                                   
       end)                                                                  
       catch err                                                             
       end  # silence!

julia> @macroexpand @avx for i = 𝒶𝓍i                                                      
           if i ∈ (axes)(var"𝛥≪rand(10)≫", 1)                                
               var"𝛥≪rand(10)≫"[i] = var"𝛥≪rand(10)≫"[i] + 𝛥ℛ[i]             
           else                                                              
               (zero)((eltype)(var"𝛥≪rand(10)≫"))                            
           end                                                               
       end  # similar output to before:
ERROR: LoadError: Don't know how to handle expression.
if i ∈ axes(var"𝛥≪rand(10)≫", 1)
    #= REPL[10]:3 =#
    var"𝛥≪rand(10)≫"[i] = var"𝛥≪rand(10)≫"[i] + 𝛥ℛ[i]
else
    #= REPL[10]:5 =#
    zero(eltype(var"𝛥≪rand(10)≫"))
end
Stacktrace:
 [1] push!(ls::LoopVectorization.LoopSet, ex::Expr, elementbytes::Int64, position::Int64)
   @ LoopVectorization ~/.julia/dev/LoopVectorization/src/graphs.jl:754
 [2] add_block!
...

Copy link
Member

@chriselrod chriselrod left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Feel free to bump the patch version.

@mcabbott
Copy link
Author

Great, done!

@codecov-io
Copy link

codecov-io commented Dec 13, 2020

Codecov Report

Merging #172 (0ef644f) into master (b156769) will increase coverage by 1.92%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
+ Coverage   92.75%   94.67%   +1.92%     
==========================================
  Files          32       32              
  Lines        4516     4511       -5     
==========================================
+ Hits         4189     4271      +82     
+ Misses        327      240      -87     
Impacted Files Coverage Δ
src/graphs.jl 88.49% <37.50%> (+1.49%) ⬆️
src/lower_memory_common.jl 97.50% <0.00%> (+1.66%) ⬆️
src/lowering.jl 97.63% <0.00%> (+2.36%) ⬆️
src/determinestrategy.jl 99.84% <0.00%> (+2.39%) ⬆️
src/reconstruct_loopset.jl 96.79% <0.00%> (+6.04%) ⬆️
src/lower_load.jl 98.36% <0.00%> (+19.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b156769...0ef644f. Read the comment docs.

@chriselrod
Copy link
Member

I'll merge and issue another release once tests pass.

@chriselrod
Copy link
Member

Perhaps add a

@test_throws LoadError @macroexpand @avx for i = 𝒶𝓍i                                                      
    if i  (axes)(var"𝛥≪rand(10)≫", 1)                                
        var"𝛥≪rand(10)≫"[i] = var"𝛥≪rand(10)≫"[i] + 𝛥ℛ[i]             
    else                                                              
        (zero)((eltype)(var"𝛥≪rand(10)≫"))                            
    end                                                               
end

?

@mcabbott
Copy link
Author

Done. Assuming that this kind of if-else is something unlikely to ever work, I guess. It was just the first one I happened to pick up for quick tests locally.

By the way, there isn't an AVX-friendly way of doing that, is there? I mean of testing some index & returning zero (or some other padding) out of bounds? Something like ifelse(i in axes(ys,1), ys[i], 0.0) will actually read out of bounds

@chriselrod chriselrod merged commit 1b48506 into JuliaSIMD:master Dec 13, 2020
@chriselrod
Copy link
Member

chriselrod commented Dec 13, 2020

Yeah, maybe it's better to have a wonkier example now, but I figured it wouldn't be that bad to change it later.

By the way, there isn't an AVX-friendly way of doing that, is there? I mean of testing some index & returning zero (or some other padding) out of bounds? Something like ifelse(i in axes(ys,1), ys[i], 0.0) will actually read out of bounds

There is, see here for an example.
If you use ? instead of ifelse, LoopVectorization will convert that into a masked load.
So you'd want to do something similar to the linked line.
Instead of

xim = i > ifirst ? x[Ipre, i-1, Ipost] : xi

you'd want

((i >= first(axes(ys,1))) & (i <= last(axes(ys,1)))) ? ys[i] : 0.0

and that should work already.

AVX has a masked move instruction:
https://www.felixcloutier.com/x86/vmaskmov
While with AVX512, it of course uses a regular move but applies a mask.
These don't read out of bounds and won't trigger segfaults.

@mcabbott
Copy link
Author

That looks great, thanks! Will try this out.

@mcabbott mcabbott deleted the silence branch December 13, 2020 14:53
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.

Spurious printing of "DiffRules._abs_deriv" when LoopVectorization.jl loaded
3 participants