Make Base.ifelse a generic function

[c42f]

Fixes #32844, in particular, to provide a way for packages to extend ifelse without needing a new package for it, such as in SciML/ModelingToolkit.jl#568. CC @ChrisRackauckas

This 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:

@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]

Inference question: Core.ifelse is handled in abstract_call_builtin() but it seems that the necessary information doesn't propagate from Base.ifelse to Core.ifelse. Why? Is this something we can expect to repair?

[ChrisRackauckas]

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.

[vtjnash]

That question is the same as #37342

[vtjnash]

#37342 is fixed now. So, do we want to merge this or close it now?

[chriselrod]

I'm in favor of merging it.

[oscardssmith]

triage says rebase and merge.

[c42f]

I've quickly rebased but it seems a deeper look is necessary because the test involving _f_ifelse_isa_ is still failing.

[chriselrod]

@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 testing

Which is what's blocking this PR.

[aviatesk]

#38905 isn't enough to enable extra constraint propagation for Base.ifelse: #38905 only "back-propagates" (callee to caller) conditional constraint, but currently we don't forward (caller to callee) it, and we need a general improvement here:

ifelselike(cnd, x, y) = cnd ? x : y
@test Base.return_types((Any,Int,)) do x, y
    ifelselike(isa(x, Int), x, y)
end |> only == Int

We can use a similar logic as #38905, and forward Conditional when it conveys a constraint imposed on other argument.
It slightly increases a complexity involved with Conditional, but I think it doesn't complicate our lattice impl and won't be that troublesome.

@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 ?

[aviatesk]

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.

[c42f]

Perfect, thanks @aviatesk, I appreciate someone who knows about inference coming to the rescue here :-)

I can rebase this branch and test again once #42529 is done (also happy for anyone else to do so if they get to it first).

[chriselrod]

Now that #42529 (comment) has merged, rebase?

[KristofferC]

Shouldn't this at least have a nanosolider run before getting merged? Also, the tests in #37343 (comment) should have been added?

[ChrisRackauckas]

Will this get back ported to the LTS?

[oscardssmith]

no. it's a feature not a bugfix

[oscardssmith]

sorry if I merged this prematurely. as I understood the issue, the inference problems were fixed in a different pr.

[oschulz]

Will this get back ported to the LTS?

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

[oscardssmith]

I'd be more inclined to agree if this pr hadn't required a change to inference to avoid regressions.

[c42f]

Thanks @oscardssmith for pushing this along.

Also, the tests in #37343 (comment) should have been added?

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?

[oscardssmith]

Presumably there is a nightly nanosoldier run somewhere that should capture if there were any major differences.

[vtjnash]

404e584#commitcomment-58840937

[vtjnash]

4c3ae20

[c42f]

Some of those performance regressions look kind of bad. Should we revert this for now?

(Ref: 4c3ae20#commitcomment-58883242)

[c42f]

@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?

[aviatesk]

I don't think there is much performance difference.
AFAIU the benchmark time isn't that reliable measure of performance (whereas any increase in memory allocation usually indicates an obvious performance regression).

I compared the performance difference of ["sparse", "matmul", ("At_mul_B", "dense 50x5, sparse 50x500 -> dense 5x500")] as an example ​with the following script:

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))))
end

But I couldn't find any effective performance difference on master and master with this PR reverted.