Make Base.ifelse a generic function#37343
Conversation
|
As an upgrade path, I created https://github.com/SciML/IfElse.jl for me and @chriselrod (and whoever else) to start usinga common form, and when something gets added to Base we can deprecate that. |
|
That question is the same as #37342 |
|
#37342 is fixed now. So, do we want to merge this or close it now? |
|
I'm in favor of merging it. |
|
triage says rebase and merge. |
73e8316 to
7953e39
Compare
|
I've quickly rebased but it seems a deeper look is necessary because the test involving |
|
@aviatesk have any ideas about this failure? using Test
@noinline _f_ifelse_isa_() = rand(Bool) ? 1 : nothing
function _g_ifelse_isa_()
x = _f_ifelse_isa_()
ifelse(isa(x, Nothing), 1, x)
end
@test Base.return_types(_g_ifelse_isa_, ()) == [Int]
generic_ifelse(a, b, c) = Core.ifelse(a, b, c)
function _g_generic_ifelse_isa_()
x = _f_ifelse_isa_()
generic_ifelse(isa(x, Nothing), 1, x)
end
@test Base.return_types(_g_generic_ifelse_isa_, ()) == [Int]I get julia> @test Base.return_types(_g_ifelse_isa_, ()) == [Int]
Test Passed
Expression: Base.return_types(_g_ifelse_isa_, ()) == [Int]
Evaluated: Any[Int64] == DataType[Int64]
julia> @test Base.return_types(_g_generic_ifelse_isa_, ()) == [Int]
Test Failed at REPL[13]:1
Expression: Base.return_types(_g_generic_ifelse_isa_, ()) == [Int]
Evaluated: Any[Union{Nothing, Int64}] == DataType[Int64]
ERROR: There was an error during testingWhich is what's blocking this PR. |
|
#38905 isn't enough to enable extra constraint propagation for ifelselike(cnd, x, y) = cnd ? x : y
@test Base.return_types((Any,Int,)) do x, y
ifelselike(isa(x, Int), x, y)
end |> only == IntWe can use a similar logic as #38905, and forward @c42f don't you mind if I rebase and add new commit onto this branch ? Or do you prefer I cut a new branch for it ? |
|
Now I think the inference improvement is better to be done separately from this PR. It will be a requirement for this PR, but just a general improvement. xref: #42529. |
|
Now that #42529 (comment) has merged, rebase? |
Allow user code to directly extend `Base.ifelse` rather than needing a special package for it.
7953e39 to
1bd99e9
Compare
|
Shouldn't this at least have a nanosolider run before getting merged? Also, the tests in #37343 (comment) should have been added? |
|
Will this get back ported to the LTS? |
|
no. it's a feature not a bugfix |
|
sorry if I merged this prematurely. as I understood the issue, the inference problems were fixed in a different pr. |
That would be nice (yes, I know, it's not a fix, but it kinda is just a "low-level" implementation change, in a way). |
|
I'd be more inclined to agree if this pr hadn't required a change to inference to avoid regressions. |
|
Thanks @oscardssmith for pushing this along.
These tests are already part of the Base test suite and more were added as part of the separate inference PR in #42529. Regarding running nanosolider, perhaps? Is there a way to run this retroactively? |
|
Presumably there is a nightly nanosoldier run somewhere that should capture if there were any major differences. |
commit c054dbc Author: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Fri Oct 29 01:31:55 2021 +0900 optimizer: eliminate allocations (JuliaLang#42833) commit 6a9737d Author: Jeff Bezanson <jeff.bezanson@gmail.com> Date: Thu Oct 28 12:23:53 2021 -0400 fix JuliaLang#42659, move `jl_coverage_visit_line` to runtime library (JuliaLang#42810) commit c762f10 Author: Marc Ittel <35898736+MarcMush@users.noreply.github.com> Date: Thu Oct 28 12:19:13 2021 +0200 change `julia` to `julia-repl` in docstrings (JuliaLang#42824) Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com> commit 9f52ec0 Author: Dilum Aluthge <dilum@aluthge.com> Date: Thu Oct 28 05:30:11 2021 -0400 CI (Buildkite): Update all rootfs images to the latest versions (JuliaLang#42802) * CI (Buildkite): Update all rootfs images to the latest versions * Re-sign all of the signed pipelines commit 404e584 Author: DilumAluthgeBot <43731525+DilumAluthgeBot@users.noreply.github.com> Date: Wed Oct 27 21:11:04 2021 -0400 🤖 Bump the Statistics stdlib from 74897fe to 5256d57 (JuliaLang#42826) Co-authored-by: Dilum Aluthge <dilum@aluthge.com> commit c74814e Author: Jeff Bezanson <jeff.bezanson@gmail.com> Date: Wed Oct 27 16:34:46 2021 -0400 reset `RandomDevice` file from `__init__` (JuliaLang#42537) This prevents us from seeing an invalid `IOStream` object from a saved system image, and also ensures the files are opened once for all threads. commit 05ed348 Author: Jeff Bezanson <jeff.bezanson@gmail.com> Date: Wed Oct 27 15:24:17 2021 -0400 only visit nonfunction_mt once when traversing method tables (JuliaLang#42821) commit d71b77d Author: DilumAluthgeBot <43731525+DilumAluthgeBot@users.noreply.github.com> Date: Tue Oct 26 20:39:08 2021 -0400 🤖 Bump the Downloads stdlib from 5f1509d to dbb0625 (JuliaLang#42811) Co-authored-by: Dilum Aluthge <dilum@aluthge.com> commit b4fddc1 Author: DilumAluthgeBot <43731525+DilumAluthgeBot@users.noreply.github.com> Date: Tue Oct 26 14:46:20 2021 -0400 🤖 Bump the Pkg stdlib from bc32103f to 26918395 (JuliaLang#42806) Co-authored-by: Dilum Aluthge <dilum@aluthge.com> commit 6a386de Author: Dilum Aluthge <dilum@aluthge.com> Date: Tue Oct 26 12:15:51 2021 -0400 CI (Buildkite): make sure to hit ignore any unencrypted repo keys, regardless of where they are located in the repository (JuliaLang#42803) commit 021a6b5 Author: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Wed Oct 27 01:08:33 2021 +0900 optimizer: clean up inlining test code (JuliaLang#42804) commit 16eb196 Merge: 21ebabf 1510eaa Author: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Tue Oct 26 23:25:41 2021 +0900 Merge pull request JuliaLang#42766 from JuliaLang/avi/42754 optimizer: fix JuliaLang#42754, inline union-split const-prop'ed sources commit 21ebabf Author: Kristoffer Carlsson <kcarlsson89@gmail.com> Date: Tue Oct 26 16:11:32 2021 +0200 simplify code loading test now that TOML files are parsed with a real TOML parser (JuliaLang#42328) commit 1510eaa Author: Shuhei Kadowaki <aviatesk@gmail.com> Date: Mon Oct 25 01:35:12 2021 +0900 optimizer: fix JuliaLang#42754, inline union-split const-prop'ed sources This commit complements JuliaLang#39754 and JuliaLang#39305: implements a logic to use constant-prop'ed results for inlining at union-split callsite. Currently it works only for cases when constant-prop' succeeded for all (union-split) signatures. > example ```julia julia> mutable struct X # NOTE in order to confuse `fieldtype_tfunc`, we need to have at least two fields with different types a::Union{Nothing, Int} b::Symbol end; julia> code_typed((X, Union{Nothing,Int})) do x, a # this `setproperty` call would be union-split and constant-prop will happen for # each signature: inlining would fail if we don't use constant-prop'ed source # since the approximated inlining cost of `convert(fieldtype(X, sym), a)` would # end up very high if we don't propagate `sym::Const(:a)` x.a = a x end |> only |> first ``` > before this commit ```julia CodeInfo( 1 ─ %1 = Base.setproperty!::typeof(setproperty!) │ %2 = (isa)(a, Nothing)::Bool └── goto #3 if not %2 2 ─ %4 = π (a, Nothing) │ invoke %1(_2::X,🅰️ :Symbol, %4::Nothing)::Any └── goto #6 3 ─ %7 = (isa)(a, Int64)::Bool └── goto #5 if not %7 4 ─ %9 = π (a, Int64) │ invoke %1(_2::X,🅰️ :Symbol, %9::Int64)::Any └── goto #6 5 ─ Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{} └── unreachable 6 ┄ return x ) ``` > after this commit ```julia CodeInfo( 1 ─ %1 = (isa)(a, Nothing)::Bool └── goto #3 if not %1 2 ─ Base.setfield!(x, :a, nothing)::Nothing └── goto #6 3 ─ %5 = (isa)(a, Int64)::Bool └── goto #5 if not %5 4 ─ %7 = π (a, Int64) │ Base.setfield!(x, :a, %7)::Int64 └── goto #6 5 ─ Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{} └── unreachable 6 ┄ return x ) ``` commit 4c3ae20 Author: Chris Foster <chris42f@gmail.com> Date: Tue Oct 26 21:48:32 2021 +1000 Make Base.ifelse a generic function (JuliaLang#37343) Allow user code to directly extend `Base.ifelse` rather than needing a special package for it. commit 2e388e3 Author: Shuhei Kadowaki <aviatesk@gmail.com> Date: Mon Oct 25 01:30:09 2021 +0900 optimizer: eliminate excessive specialization in inlining code This commit includes several code quality improvements in inlining code: - eliminate excessive specializations around: * `item::Pair{Any, Any}` constructions * iterations on `Vector{Pair{Any, Any}}` - replace `Pair{Any, Any}` with new, more explicit data type `InliningCase` - remove dead code
|
Some of those performance regressions look kind of bad. Should we revert this for now? |
|
@aviatesk do you have any thoughts about the performance regressions from the nanosolider run and whether there's anything that could be done about them in the short term? |
|
I don't think there is much performance difference. I compared the performance difference of include(normpath(ENV["JULIA_PKG_DEVDIR"], "BaseBenchmarks", "src", "utils", "RandUtils.jl"))
using .RandUtils
using BenchmarkTools
using SparseArrays
using LinearAlgebra
using LinearAlgebra: *, mul!
function allocmats_ds(om, ok, on, s, nnzc, T)
m, k, n = map(x -> Int(s*x), (om, ok, on))
densemat, sparsemat = samerand(T, m, k), samesprand(T, k, n, nnzc/k)
tdensemat = transpose!(similar(densemat, reverse(size(densemat))), densemat)
tsparsemat = transpose!(similar(sparsemat, reverse(size(sparsemat))), sparsemat)
destmat = similar(densemat, m, n)
return m, k, n, destmat,
densemat, sparsemat,
tdensemat, tsparsemat
end
for (om, ok, on) in (# order of matmul dimensions m, k, and n
(10^2, 10^2, 10^2), # dense square * sparse square -> dense square
(10^1, 10^1, 10^3), # dense square * sparse short -> dense short
(10^2, 10^2, 10^1), # dense square * sparse tall -> dense tall
(10^1, 10^3, 10^3), # dense short * sparse square -> dense short
(10^1, 10^2, 10^3), # dense short * sparse short -> dense short
(10^1, 10^3, 10^2), # dense short * sparse tall -> dense short
(10^3, 10^1, 10^1), # dense tall * sparse square -> dense tall
(10^2, 10^1, 10^2), # dense tall * sparse short -> dense square
) # the preceding descriptions apply to dense-sparse matmul without
# any transpositions. other cases are described below
m, k, n, destmat, densemat, sparsemat, tdensemat, tsparsemat = allocmats_ds(om, ok, on, 1/2, 4, Float64)
display(@benchmark *($densemat, $sparsemat))
display(@benchmark *($densemat, $(Transpose(tsparsemat))))
endBut I couldn't find any effective performance difference on |
Allow user code to directly extend `Base.ifelse` rather than needing a special package for it.
Allow user code to directly extend `Base.ifelse` rather than needing a special package for it.
Fixes #32844, in particular, to provide a way for packages to extend
ifelsewithout needing a new package for it, such as in SciML/ModelingToolkit.jl#568. CC @ChrisRackauckasThis is a WIP because it regresses some inference support for
ifelse(likely undoing some of the work from #27068), in particular, the following test case now fails:Inference question:
Core.ifelseis handled inabstract_call_builtin()but it seems that the necessary information doesn't propagate fromBase.ifelsetoCore.ifelse. Why? Is this something we can expect to repair?