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

Improve effect analysis of bitshifts by non Int and UInt Integers #47567

Merged
merged 7 commits into from
Nov 17, 2022

Conversation

oscardssmith
Copy link
Member

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

base/operators.jl Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <[email protected]>
@giordano
Copy link
Contributor

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.

test/int.jl Outdated Show resolved Hide resolved
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.

base/operators.jl Show resolved Hide resolved
@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
Contributor

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
Sponsor 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.

None yet

7 participants