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

Try out lowered IR validation. #331

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented May 18, 2022

Trying out the integration from #311 to at least validate lowered IR and, e.g., find references to non-const globals.

@maleadt
Copy link
Member Author

maleadt commented May 18, 2022

The errors also aren't particularly nice...

ERROR: GPU compilation of kernel #kernel() failed
KernelError: using mutable global: Main.b

Try inspecting the generated code with any of the @device_code_... macros.

Stacktrace:
  [1] (::GPUCompiler.var"#validate#54"{GPUCompiler.GPUInterpreter})(x::GlobalRef)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/jlgen.jl:239
  [2] _broadcast_getindex_evalf
    @ ./broadcast.jl:670 [inlined]
  [3] _broadcast_getindex
    @ ./broadcast.jl:643 [inlined]
  [4] getindex
    @ ./broadcast.jl:597 [inlined]
  [5] copyto_nonleaf!(dest::Vector{Nothing}, bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Tuple{Base.OneTo{Int64}}, GPUCompiler.var"#validate#54"{GPUCompiler.GPUInterpreter}, Tuple{Base.Broadcast.Extruded{Vector{Any}, Tuple{Bool}, Tuple{Int64}}}}, iter::Base.OneTo{Int64}, state::Int64, count::Int64)
    @ Base.Broadcast ./broadcast.jl:1055
  [6] copy(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Tuple{Base.OneTo{Int64}}, GPUCompiler.var"#validate#54"{GPUCompiler.GPUInterpreter}, Tuple{Vector{Any}}})
    @ Base.Broadcast ./broadcast.jl:907
  [7] materialize
    @ ./broadcast.jl:860 [inlined]
  [8] (::GPUCompiler.var"#validate#54"{GPUCompiler.GPUInterpreter})(x::Expr)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/jlgen.jl:230
  [9] _broadcast_getindex_evalf
    @ ./broadcast.jl:670 [inlined]
 [10] _broadcast_getindex
    @ ./broadcast.jl:643 [inlined]
 [11] getindex
    @ ./broadcast.jl:597 [inlined]
 [12] copy(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Tuple{Base.OneTo{Int64}}, GPUCompiler.var"#validate#54"{GPUCompiler.GPUInterpreter}, Tuple{Vector{Any}}})
    @ Base.Broadcast ./broadcast.jl:899
 [13] materialize
    @ ./broadcast.jl:860 [inlined]
 [14] validate_globalrefs(interp::GPUCompiler.GPUInterpreter, mi::Core.MethodInstance, src::Core.CodeInfo)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/jlgen.jl:251
 [15] Core.Compiler.InferenceState(result::Core.Compiler.InferenceResult, cached::Symbol, interp::GPUCompiler.GPUInterpreter)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/jlgen.jl:223
 [16] typeinf
    @ ./compiler/typeinfer.jl:9 [inlined]
 [17] typeinf_type(interp::GPUCompiler.GPUInterpreter, method::Method, atype::Any, sparams::Core.SimpleVector)
    @ Core.Compiler ./compiler/typeinfer.jl:979
 [18] return_type(m::Core.MethodMatch; interp::GPUCompiler.GPUInterpreter)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/validation.jl:17

At the least we'd need a valid stack trace. Maybe we should drop some marker that's easy to pick up during LLVM IR validation? Seems hacky though.

@maleadt
Copy link
Member Author

maleadt commented May 18, 2022

We can do a little better:

julia> empty!(GPUCompiler.GLOBAL_CI_CACHE); main()
ERROR: InvalidIRError: compiling kernel #kernel() resulted in invalid LLVM IR
Reason: unsupported using mutable global: Main.b
Stacktrace:
 [1] kernel()
   @ Main ~/Julia/pkg/GPUCompiler/wip.jl:20
Hint: catch this exception as `err` and call `code_typed(err; interactive = true)` to introspect the erronous code
Stacktrace:
  [1] validate_globalrefs(interp::GPUCompiler.GPUInterpreter, mi::Core.MethodInstance, src::Core.CodeInfo)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/jlgen.jl:266
  [2] Core.Compiler.InferenceState(result::Core.Compiler.InferenceResult, cached::Symbol, interp::GPUCompiler.GPUInterpreter)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/jlgen.jl:223
  [3] typeinf
    @ ./compiler/typeinfer.jl:9 [inlined]
  [4] typeinf_type(interp::GPUCompiler.GPUInterpreter, method::Method, atype::Any, sparams::Core.SimpleVector)
    @ Core.Compiler ./compiler/typeinfer.jl:979
  [5] return_type(m::Core.MethodMatch; interp::GPUCompiler.GPUInterpreter)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/validation.jl:17
  [6] check_method(job::CompilerJob)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/validation.jl:36
  [7] macro expansion
    @ ~/Julia/depot/packages/TimerOutputs/LDL7n/src/TimerOutput.jl:252 [inlined]
  [8] macro expansion
    @ ~/Julia/pkg/GPUCompiler/src/driver.jl:145 [inlined]
  [9] emit_julia(job::CompilerJob)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/utils.jl:64
 [10] codegen(output::Symbol, job::CompilerJob; libraries::Bool, deferred_codegen::Bool, optimize::Bool, strip::Bool, validate::Bool, only_entry::Bool, parent_job::Nothing, ctx::Nothing)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/driver.jl:86
 [11] compile(target::Symbol, job::CompilerJob; libraries::Bool, deferred_codegen::Bool, optimize::Bool, strip::Bool, validate::Bool, only_entry::Bool, ctx::Nothing)
    @ GPUCompiler ~/Julia/pkg/GPUCompiler/src/driver.jl:41
 [12] compile
    @ ~/Julia/pkg/GPUCompiler/src/driver.jl:37 [inlined]
 [13] main()
    @ Main ~/Julia/pkg/GPUCompiler/wip.jl:30
 [14] top-level scope
    @ REPL[22]:1

I wonder if dropping an error marker and picking it up during codegen is more 'true' to the original runtime semantics of errors (because at least we'd be able to reconstruct more of the backtrace). But the IR isn't trivial to rewrite (to insert an error marker) at that point.

@vchuravy
Copy link
Member

I normally for arguing about turning these into runtime errors but the current workflow is a bit painful and requires a restart to get the actual error.

@maleadt
Copy link
Member Author

maleadt commented May 19, 2022

We can move towards run-time errors, but:

  • do we then also want this for other non-compileable constructs (like calls to the Julia runtime, etc)?
  • are there utilities for rewriting lowered IR? just replacing the GlobalRef with an Expr(:call, :error) or so already requires inserting a new expression and rewriting references.

@vchuravy
Copy link
Member

rewriting lowered IR

I use the Cassette tooling for that, but we would need to pull that out. Unsure what IRTools has these days

@maleadt
Copy link
Member Author

maleadt commented May 19, 2022

I'm also not convinced we want to move to runtime errors for everything...

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #331 (c2c8a59) into master (fa386cc) will decrease coverage by 1.72%.
The diff coverage is 31.70%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
- Coverage   85.63%   83.91%   -1.73%     
==========================================
  Files          23       23              
  Lines        2353     2281      -72     
==========================================
- Hits         2015     1914     -101     
- Misses        338      367      +29     
Impacted Files Coverage Δ
src/interface.jl 82.75% <ø> (-1.34%) ⬇️
src/validation.jl 83.54% <26.66%> (-13.64%) ⬇️
src/jlgen.jl 77.84% <40.00%> (-2.28%) ⬇️
src/driver.jl 91.52% <100.00%> (-0.37%) ⬇️
src/optim.jl 81.53% <0.00%> (-7.52%) ⬇️
src/reflection.jl 76.37% <0.00%> (-3.94%) ⬇️
src/debug.jl 91.17% <0.00%> (-0.50%) ⬇️
src/reflection_compat.jl 53.75% <0.00%> (-0.47%) ⬇️
... and 10 more

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 fa386cc...c2c8a59. Read the comment docs.

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.

2 participants