Core.ifelse calls to avoid invalidations from defining custom Base.ifelse methods

[chriselrod]

Addresses ifelse invalidations reported here:
SciML/Static.jl#77 (comment)

After this PR:

julia> using SnoopCompileCore

julia> invals = @snoopr using Static
104-element Vector{Any}:
  MethodInstance for to_indices(::AbstractArray, ::Tuple{Any, CartesianIndex{0}})
 1
  MethodInstance for setindex!(::AbstractArray, ::Any, ::Any, ::CartesianIndex{0})
 2
  MethodInstance for to_indices(::AbstractArray, ::Any, ::Tuple{Any, CartesianIndex{0}})
  "jl_method_table_insert"
  to_indices(A, inds, I::Tuple{AbstractArray{NDIndex{N, J}}, Vararg{Any}}) where {N, J} @ Static ~/.julia/dev/Static/src/Static.jl:919
  "jl_method_table_insert"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
 ⋮
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for promote_rule(::Type, ::Type)
  "invalidate_mt_cache"
  MethodInstance for Base._cutdim(::Any, ::CartesianIndex{0})
 1
  MethodInstance for Base.IteratorsMD.split(::Any, ::Val{0})
  "jl_method_table_insert"
  split(i::NDIndex, V::Val) @ Static ~/.julia/dev/Static/src/Static.jl:855
  "jl_method_table_insert"
  MethodInstance for ∘(::Function, ::Function)
  "invalidate_mt_cache"

julia> using SnoopCompileCore

julia> using SnoopCompile

julia> tree = invalidation_trees(invals)
2-element Vector{SnoopCompile.MethodInvalidations}:
 inserting split(i::NDIndex, V::Val) @ Static ~/.julia/dev/Static/src/Static.jl:855 invalidated:
   backedges: 1: superseding split(t, V::Val) @ Base.IteratorsMD multidimensional.jl:480 with MethodInstance for Base.IteratorsMD.split(::Any, ::Val{0}) (1 children)
   44 mt_cache

 inserting to_indices(A, inds, I::Tuple{AbstractArray{NDIndex{N, J}}, Vararg{Any}}) where {N, J} @ Static ~/.julia/dev/Static/src/Static.jl:919 invalidated:
   backedges: 1: superseding to_indices(A, inds, I::Tuple{Any, Vararg{Any}}) @ Base indices.jl:352 with MethodInstance for to_indices(::AbstractArray, ::Any, ::Tuple{Any, CartesianIndex{0}}) (2 children)

[chriselrod]

Alternatively, I could call Core.ifelse in these places instead.

[oscardssmith]

Neither of these approaches seem great, but there might not be a better solution.

[chriselrod]

Perhaps I should do Bool(x)::Bool instead, so things like Static.False would still work here.

This breaks things returning VectorizationBase.Mask being used here, which I am okay with.
@shashi does this impact anything in the Symbolics ecosystem?
Note that these locations aren't going to be hit if methods like max are defined preventing these from being reached.

[Tokazama]

Thanks for taking the lead on this. I looked at it and wasn't sure what the best path forward was. It seems like we shouldn't have this much trouble inferring Bool.

[chriselrod]

Jeff suggested Core.ifelse instead of type asserts.

[JeffBezanson]

It seems like we shouldn't have this much trouble inferring Bool.

The trouble is we don't have a way to enforce types like isless(::Any, ::Any)::Bool, but maybe we will one day.

[chriselrod]

The trouble is we don't have a way to enforce types like isless(::Any, ::Any)::Bool, but maybe we will one day.

That would be nice.
But, for now, I'm using Core.ifelse here as you suggested.

[aviatesk]

It sounds reasonable to use Core.ifelse in place of ifelse(c, x, y) where the types of c/x/y are known to be such types that are owned by Base, but otherwise it sounds against Julia's design pattern.

E.g. max(x, y) = Core.ifelse(isless(y, x), x, y) disallows the possibility to overload Base.ifelse(c, x::S, y::T) to customize max(::S, ::T)/min(::S, ::T), where S and T are owned by users.

But it seems reasonable to me to define max(x::Float64, y::Float64) = Core.ifelse(isless(y, x), x, y) as users aren't supposed to override the definitions of isless and ifelse for Float64 types.

[KristofferC]

E.g. max(x, y) = Core.ifelse(isless(y, x), x, y) disallows the possibility to overload Base.ifelse(c, x::S, y::T) to customize max(::S, ::T)

On the other hand, you probably shouldn't rely on overloading ifelse in this case since someone might rewrite this to islexx(y,x) ? x : y and if you needed ifelse overload for this to work correctly, it would all of a sudden be broken.

[Tokazama]

E.g. max(x, y) = Core.ifelse(isless(y, x), x, y) disallows the possibility to overload Base.ifelse(c, x::S, y::T) to customize max(::S, ::T)

On the other hand, you probably shouldn't rely on overloading ifelse in this case since someone might rewrite this to islexx(y,x) ? x : y and if you needed ifelse overload for this to work correctly, it would all of a sudden be broken.

IIRC, the reason Base.ifelse was overloaded was for internal purposes in VectorizationBase, not public usage. The broader issue we're trying to address is that underlying we keep running into a fresh batch of invalidations. For example, we're now trying to address a huge amount of invalidations with ! now SciML/Static.jl#78.

I kind of agree that we can (maybe should) revert overloading Base.ifelse to get by, but given that this is just one of many related issues it seems like we need something to help prevent this sort of thing from popping up with every release.