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

Inference regression with === nothing #26417

Closed
nalimilan opened this issue Mar 11, 2018 · 7 comments · Fixed by #27972
Closed

Inference regression with === nothing #26417

nalimilan opened this issue Mar 11, 2018 · 7 comments · Fixed by #27972
Labels
compiler:inference Type inference missing data Base.missing and related functionality regression Regression in behavior compared to a previous version
Milestone

Comments

@nalimilan
Copy link
Member

The return type f(1) used to be inferred as Int on 0.6.2, but on master it's inferred as Union{Int,Nothing}. It still works when using isa Nothing though.

f(x) = x === nothing ? nothing : 1
@nalimilan nalimilan added regression Regression in behavior compared to a previous version compiler:inference Type inference missing data Base.missing and related functionality labels Mar 11, 2018
nalimilan added a commit to JuliaData/CategoricalArrays.jl that referenced this issue Mar 11, 2018
Use isa instead of === to work an inference regression (JuliaLang/julia#26417).
nalimilan added a commit to JuliaData/CategoricalArrays.jl that referenced this issue Mar 11, 2018
Use isa instead of === to work an inference regression (JuliaLang/julia#26417).
nalimilan added a commit to JuliaData/CategoricalArrays.jl that referenced this issue Mar 11, 2018
Use isa instead of === to work an inference regression (JuliaLang/julia#26417).
@JeffBezanson
Copy link
Member

Maybe related to #26339?

nalimilan added a commit to JuliaData/CategoricalArrays.jl that referenced this issue Mar 14, 2018
Use isa instead of === to work an inference regression (JuliaLang/julia#26417).
@JeffBezanson
Copy link
Member

I tried this patch, which much to my dismay does not work:

--- a/base/compiler/abstractinterpretation.jl
+++ b/base/compiler/abstractinterpretation.jl
@@ -546,13 +546,15 @@ function abstract_call(@nospecialize(f), fargs::Union{Tuple{},Vector{Any}}, argt
                 # if doing a comparison to a singleton, consider returning a `Conditional` instead
                 if isa(aty, Const) && isa(b, Slot)
                     if isdefined(typeof(aty.val), :instance) # can only widen a if it is a singleton
-                        return Conditional(b, aty, typesubtract(widenconst(bty), typeof(aty.val)))
+                        trueT = (isa(rt,Const) && rt.val === false) ? Bottom : aty
+                        return Conditional(b, trueT, typesubtract(widenconst(bty), typeof(aty.val)))
                     end
                     return isa(rt, Const) ? rt : Conditional(b, aty, bty)
                 end
                 if isa(bty, Const) && isa(a, Slot)
                     if isdefined(typeof(bty.val), :instance) # same for b
-                        return Conditional(a, bty, typesubtract(widenconst(aty), typeof(bty.val)))
+                        trueT = (isa(rt,Const) && rt.val === false) ? Bottom : bty
+                        return Conditional(a, trueT, typesubtract(widenconst(aty), typeof(bty.val)))
                     end
                     return isa(rt, Const) ? rt : Conditional(a, bty, aty)
                 end

It fails during sysimg build with:

error during bootstrap:
LoadError("sysimg.jl", 445, LoadError("docs/basedocs.jl", 10, MethodError(convert, (String, 0x0000000000000058), 0x00000000000038bc)))
rec_backtrace at /home/bezanson/src/julia/src/stackwalk.c:94
record_backtrace at /home/bezanson/src/julia/src/task.c:246
jl_throw at /home/bezanson/src/julia/src/task.c:577
jl_method_error_bare at /home/bezanson/src/julia/src/gf.c:1608
jl_method_error at /home/bezanson/src/julia/src/gf.c:1626
jl_lookup_generic_ at /home/bezanson/src/julia/src/gf.c:2117 [inlined]
jl_apply_generic at /home/bezanson/src/julia/src/gf.c:2137
convert at ./essentials.jl:248
convert at ./some.jl:21
unknown function (ip: 0x7fb6fd33fc84)
jl_fptr_trampoline at /home/bezanson/src/julia/src/gf.c:1823
jl_apply_generic at /home/bezanson/src/julia/src/gf.c:2140
setproperty! at ./sysimg.jl:19
unknown function (ip: 0x7fb6fd33fc4b)
jl_fptr_trampoline at /home/bezanson/src/julia/src/gf.c:1823
jl_apply_generic at /home/bezanson/src/julia/src/gf.c:2140
popfirst! at ./iterators.jl:1078 [inlined]
iterate at ./iterators.jl:1085 [inlined]
iterate at ./iterators.jl:1085 [inlined]
join at ./strings/io.jl:246
unknown function (ip: 0x7fb6fd33f99d)
jl_fptr_trampoline at /home/bezanson/src/julia/src/gf.c:1823
jl_apply_generic at /home/bezanson/src/julia/src/gf.c:2140
print at ./version.jl:77
#print_to_string#337 at ./strings/io.jl:122
jl_fptr_trampoline at /home/bezanson/src/julia/src/gf.c:1823
jl_apply_generic at /home/bezanson/src/julia/src/gf.c:2140
print_to_string at ./strings/io.jl:110 [inlined]
string at ./strings/io.jl:141

Interestingly, even just disabling the code to return Conditional for calls to === also fails, which makes me think something else somewhere is very wrong.

@JeffBezanson JeffBezanson added this to the 1.0.x milestone Jun 12, 2018
@JeffBezanson
Copy link
Member

If I disable the Conditional code for === I get this:

julia: /home/jeff/src/julia/deps/srccache/llvm-6.0.0/include/llvm/Support/Casting.h:106: static bool llvm::isa_impl_cl<To, const From*>::doit(const From*) [with To = llvm::BranchInst; From = llvm::TerminatorInst]: Assertion `Val && "isa<> used on a null pointer"' failed.

signal (6): Aborted
in expression starting at show.jl:644
raise at /build/buildd/eglibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56
abort at /build/buildd/eglibc-2.19/stdlib/abort.c:89
__assert_fail_base at /build/buildd/eglibc-2.19/assert/assert.c:92
__assert_fail at /build/buildd/eglibc-2.19/assert/assert.c:101
_ZN4llvm11isa_impl_clINS_10BranchInstEPKNS_14TerminatorInstEE4doitES4_.part.294 at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZN4llvm14GetIfConditionEPNS_10BasicBlockERS1_S2_ at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZL19FoldTwoEntryPHINodePN4llvm7PHINodeERKNS_19TargetTransformInfoERKNS_10DataLayoutE at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZN4llvm11simplifyCFGEPNS_10BasicBlockERKNS_19TargetTransformInfoERKNS_18SimplifyCFGOptionsEPNS_15SmallPtrSetImplIS1_EE at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZL22iterativelySimplifyCFGRN4llvm8FunctionERKNS_19TargetTransformInfoERKNS_18SimplifyCFGOptionsE at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZN12_GLOBAL__N_115CFGSimplifyPass13runOnFunctionERN4llvm8FunctionE at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZN4llvm13FPPassManager13runOnFunctionERNS_8FunctionE at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZN4llvm13FPPassManager11runOnModuleERNS_6ModuleE at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
_ZN4llvm6legacy15PassManagerImpl3runERNS_6ModuleE at /home/jeff/src/julia/usr/bin/../lib/libLLVM-6.0.so (unknown line)
operator() at /home/jeff/src/julia/src/jitlayers.cpp:485
addModule at /home/jeff/src/julia/usr/include/llvm/ExecutionEngine/Orc/IRCompileLayer.h:57 [inlined]

@vtjnash
Copy link
Member

vtjnash commented Jul 6, 2018

1e0ce67 changed the definition of in to use === instead of ismissing, which now causes Core.Inference to perform poorly (The in function is used very heavily by the new optimizer (via is_relevant_expr), so it's quite critical to overall performance), so making this a priority issue

@JeffBezanson
Copy link
Member

In the meantime, I'll switch in back to ismissing, and we can also unroll the in call in is_relevant_expr.

@vtjnash
Copy link
Member

vtjnash commented Jul 6, 2018

The code using in is already very good, as long as inference doesn't fail

vtjnash added a commit that referenced this issue Jul 6, 2018
And re-tighten Inference information for Const boolean converted to Conditional

fix #26417
fix #26339
vtjnash added a commit that referenced this issue Jul 6, 2018
And re-tighten Inference information for Const boolean converted to Conditional

fix #26417
fix #26339
@nalimilan
Copy link
Member Author

FWIW I'd rather use ismissing where it's fast anyway, since it could be used by a custom missing value type (which would also carry information about the reason for missingness).

vtjnash added a commit that referenced this issue Jul 7, 2018
And re-tighten Inference information for Const boolean converted to Conditional

fix #26417
fix #26339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference missing data Base.missing and related functionality regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants