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

Fix codegen not handling invoke exprs with Codeinstances iwith jl_fptr_sparam_addr invoke types. #56817

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

gbaraldi
Copy link
Member

@gbaraldi gbaraldi commented Dec 12, 2024

fixes #56739
I didn't succeed in making a test for this. The sole trigger seems to be

using HMMGradients
T = Float32
A = T[0.0 1.0 0.0; 0.0 0.5 0.5; 1.0 0.0 0.0]
t2tr = Dict{Int,Vector{Int}}[Dict(1 => [2]),]
t2IJ= HMMGradients.t2tr2t2IJ(t2tr)

Nt = length(t2tr)+1
Ns = size(A,1)
y = rand(T,Nt,Ns)

c = rand(Float32, Nt)
beta = backward(Nt,A,c,t2IJ,y)
gamma = posterior(Nt,t2IJ,A,y)

in @oscardssmith memorynew PR

One other option is to have the builtin handle receiving a CI. That might make the code cleaner and does handle the case where we receive a dynamic CI (is that even a thing)

@gbaraldi gbaraldi requested review from vtjnash and Keno December 12, 2024 19:18
@vtjnash
Copy link
Member

vtjnash commented Dec 12, 2024

SGTM. I think we'll delete that other code path (make it an assertion) anyways, since the dynamic version is unused and unnecessary (now that the invoke function supports this instead)

@oscardssmith oscardssmith added compiler:codegen Generation of LLVM IR and native code bugfix This change fixes an existing bug labels Dec 12, 2024
@oscardssmith
Copy link
Member

oscardssmith commented Dec 12, 2024

Turns out this is very reproducable on nightly without the memorynew PR: (it works fine on 1.11 though)

julia> f(a) where {T} = a
WARNING: method definition for f at REPL[1]:1 declares type variable T but does not use it.
f (generic function with 1 method)

julia> f(1)
1

julia> g(x) = @noinline f(x)
g (generic function with 1 method)

julia> g(1)

[1492307] signal 11 (1): Segmentation fault
in expression starting at REPL[4]:1
_jl_invoke at /home/oscardssmith/julia/src/gf.c:3340 [inlined]
ijl_invoke at /home/oscardssmith/julia/src/gf.c:3369
g at ./REPL[3]:1
unknown function (ip: 0x7ac2a408e585) at (unknown file)
jl_apply at /home/oscardssmith/julia/src/julia.h:2233 [inlined]
do_call at /home/oscardssmith/julia/src/interpreter.c:125
eval_value at /home/oscardssmith/julia/src/interpreter.c:243
eval_stmt_value at /home/oscardssmith/julia/src/interpreter.c:194 [inlined]
eval_body at /home/oscardssmith/julia/src/interpreter.c:705
jl_interpret_toplevel_thunk at /home/oscardssmith/julia/src/interpreter.c:898
jl_toplevel_eval_flex at /home/oscardssmith/julia/src/toplevel.c:1068
__repl_entry_eval_expanded_with_loc at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:341
jl_apply at /home/oscardssmith/julia/src/julia.h:2233 [inlined]
jl_f__call_latest at /home/oscardssmith/julia/src/builtins.c:883
#invokelatest#1 at ./essentials.jl:1056 [inlined]
invokelatest at ./essentials.jl:1052 [inlined]
toplevel_eval_with_hooks at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:348
toplevel_eval_with_hooks at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:352
toplevel_eval_with_hooks at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:352
toplevel_eval_with_hooks at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:352
toplevel_eval_with_hooks at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:345 [inlined]
eval_user_input at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:370
repl_backend_loop at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:482
#start_repl_backend#41 at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:467
start_repl_backend at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:464 [inlined]
#run_repl#48 at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:690
run_repl at /home/oscardssmith/julia/usr/share/julia/stdlib/v1.12/REPL/src/REPL.jl:676
jfptr_run_repl_22939 at /home/oscardssmith/julia/usr/share/julia/compiled/v1.12/REPL/u0gqU_qk7sj.so (unknown line)
run_std_repl at ./client.jl:490
jfptr_run_std_repl_108302 at /home/oscardssmith/julia/usr/lib/julia/sys.so (unknown line)
jl_apply at /home/oscardssmith/julia/src/julia.h:2233 [inlined]
jl_f__call_latest at /home/oscardssmith/julia/src/builtins.c:883
#invokelatest#1 at ./essentials.jl:1056 [inlined]
invokelatest at ./essentials.jl:1052 [inlined]
run_main_repl at ./client.jl:511
repl_main at ./client.jl:593 [inlined]
_start at ./client.jl:568
jfptr__start_108359 at /home/oscardssmith/julia/usr/lib/julia/sys.so (unknown line)
jl_apply at /home/oscardssmith/julia/src/julia.h:2233 [inlined]
true_main at /home/oscardssmith/julia/src/jlapi.c:922
jl_repl_entrypoint at /home/oscardssmith/julia/src/jlapi.c:1081
main at /home/oscardssmith/julia/cli/loader_exe.c:58
unknown function (ip: 0x7ac58162814f) at /lib/x86_64-linux-gnu/libc.so.6
__libc_start_main at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
_start at ./julia (unknown line)
Allocations: 1231993 (Pool: 1231898; Big: 95); GC: 2
Segmentation fault

@giordano
Copy link
Contributor

That looks like #56739

@oscardssmith
Copy link
Member

that would be because it is :)

@topolarity
Copy link
Member

the dynamic version is unused and unnecessary (now that the invoke function supports this instead)

Does that mean we make Expr(:invoke, ::CodeInstance) require the argument to be a constant in the expression (literally - not just an IR value with ::Const type)? Can we document that and add it to the IR verifier?

@giordano giordano added the needs tests Unit tests are required for this change label Dec 12, 2024
@gbaraldi gbaraldi removed the needs tests Unit tests are required for this change label Dec 13, 2024
@Keno
Copy link
Member

Keno commented Dec 13, 2024

Does that mean we make Expr(:invoke, ::CodeInstance) require the argument to be a constant in the expression (literally - not just an IR value with ::Const t

Yes, that has always been required.

@oscardssmith
Copy link
Member

should we run benchmarks on this to make sure it isn't deoptimizing too many of our calls?

@topolarity
Copy link
Member

Yes, that has always been required.

Doesn't that mean we're fixing the wrong bug?

@gbaraldi
Copy link
Member Author

While I added an extra check to the apparently unused dynamic case. The actual fix is that we no longer try to emit specsig calls for MIs with unbound sparams.

@gbaraldi gbaraldi merged commit ece1c70 into master Dec 13, 2024
5 of 7 checks passed
@gbaraldi gbaraldi deleted the gb/ci-invoke branch December 13, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in Automa.jl tests on master
6 participants