Skip to content

Conversation

oscardssmith
Copy link
Member

this improves the effect analysis of things like UInt32 >> Int32

Co-authored-by: Jameson Nash <[email protected]>
@giordano
Copy link
Member

Can we have a more descriptive title than "mess with..."? Also, can the claim about improved effect analysis be backed up with tests?

@oscardssmith oscardssmith changed the title mess with bitshift logic Improve effect analysis of bitshifts by non Int and UInt Integers Nov 15, 2022
@oscardssmith
Copy link
Member Author

tests added.

Co-authored-by: Thomas Christensen <[email protected]>
@oscardssmith oscardssmith added compiler:effects effect analysis backport 1.9 Change should be backported to release-1.9 labels Nov 15, 2022
@oscardssmith
Copy link
Member Author

adding backport-1.9 because this is needed for #47501 to be nothrow.

@oscardssmith oscardssmith merged commit e5ed5be into JuliaLang:master Nov 17, 2022
@oscardssmith oscardssmith deleted the mess-with-bitshift-logic branch November 17, 2022 13:57
@quinnj
Copy link
Member

quinnj commented Nov 18, 2022

Did this perhaps break BitIntegers package? Seeing this on a dependent package nightly CI failure:

fatal: error thrown and no exception handler available.
MethodError(f=Base.:(<<), args=(0x00000000, 24), world=0x000000000000826b)
jl_method_error_bare at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:1942
jl_method_error at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:1960
jl_lookup_generic_ at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2688 [inlined]
ijl_apply_generic at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2703
Char at ./char.jl:162
AbstractChar at ./char.jl:48
parseint_iterate at ./parse.jl:51
parseint_preamble at ./parse.jl:58
tryparse_internal at ./parse.jl:107
#tryparse#509 at ./parse.jl:237
tryparse at ./parse.jl:235
repl_color at ./client.jl:17
error_color at ./client.jl:22
display_error at ./client.jl:110
unknown function (ip: 0x7f3b1177af46)
_jl_invoke at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2525 [inlined]
ijl_apply_generic at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2707
display_error at ./client.jl:114
unknown function (ip: 0x7f3b1177a852)
_jl_invoke at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2525 [inlined]
ijl_apply_generic at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2707
jl_apply at /cache/build/default-amdci5-6/julialang/julia-master/src/julia.h:1870 [inlined]
jl_f__call_latest at /cache/build/default-amdci5-6/julialang/julia-master/src/builtins.c:774
#invokelatest#2 at ./essentials.jl:816 [inlined]
invokelatest at ./essentials.jl:813 [inlined]
_start at ./client.jl:524
jfptr__start_[470](https://github.com/RelationalAI/rai-sdk-julia/actions/runs/3496167789/jobs/5854156556#step:3:491)94.clone_1 at /opt/hostedtoolcache/julia/nightly/x64/lib/julia/sys.so (unknown line)
_jl_invoke at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2525 [inlined]
ijl_apply_generic at /cache/build/default-amdci5-6/julialang/julia-master/src/gf.c:2707
jl_apply at /cache/build/default-amdci5-6/julialang/julia-master/src/julia.h:1870 [inlined]
true_main at /cache/build/default-amdci5-6/julialang/julia-master/src/jlapi.c:573
jl_repl_entrypoint at /cache/build/default-amdci5-6/julialang/julia-master/src/jlapi.c:717
main at /cache/build/default-amdci5-6/julialang/julia-master/cli/loader_exe.c:58
__libc_start_main at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x401098)
ERROR: LoadError: Failed to precompile BitIntegers [c3b6d118-76ef-56ca-8cc7-ebb389d030a1] to /home/runner/.julia/compiled/v1.10/BitIntegers/jl_6QfzKk.
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] compilecache(pkg::Base.PkgId, path::String, internal_stderr::IO, internal_stdout::IO, keep_loaded_modules::Bool)
    @ Base ./loading.jl:1820

@giordano
Copy link
Member

Maybe the "mess with..." title was more accurate after all

@oscardssmith
Copy link
Member Author

grr. I guess we have to revert this and make effect analysis smart enough to do this itself. @aviatesk does that seem possible to you?

@oscardssmith oscardssmith removed the backport 1.9 Change should be backported to release-1.9 label Nov 18, 2022
@gbaraldi
Copy link
Member

My pr has assume effects for both things currently, because with constprop LLVM figures out it can't throw anyway. So it's not a blocker or anything. It would just be nice

@aviatesk
Copy link
Member

grr. I guess we have to revert this and make effect analysis smart enough to do this itself.

What's the problem with this PR? Why not use @assume_effects?

@oscardssmith
Copy link
Member Author

the hard part of this PR is that before it UInt32 >> Int32 was hitting the method for Integer>>Integer which has a throw statement that would not be reached, but that the effects system couldn't tell wouldn't be reached.

@quinnj
Copy link
Member

quinnj commented Dec 6, 2022

So......should we open a new issue to track that BitIntegers.jl is still broken (can't load) from this PR? Or should we revert? It's making testing latest w/ a lot packages not work.

@oscardssmith
Copy link
Member Author

I guess I'll revert. It will make me sad, but I don't know of a better option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:effects effect analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants