Refactor null_safe_op to workaround codegen changes

[TotalVerb]

There have been codegen / inference changes since #16961 was merged that result in substantial (~40x) regressions in trivial Nullable broadcasts.

See https://discourse.julialang.org/t/performance-of-dot-operator-on-nullables/3071 for a report of the regressions.

This PR works around those changes by combining all parameters for null_safe_op into a single Type{<:Tuple} argument, which avoids the splat of (::Type{<:Tuple}).parameters which had previously confounded codegen (and would not be eliminated even when the function is declared @pure.)

Since this is a performance only change it should go into 0.6.

[TotalVerb]

cc @nalimilan
cc @davidanthoff

[ararslan]

@nanosoldier runbenchmarks("nullable", vs=":master")

Edit: Hm, maybe that's not the right way to invoke the nullable benchmarks...

[vtjnash]

eltype(a) is not pure, so it's likely a bad idea to mark this entire stack of calls as @pure (it seems the same applies to eltypestuple also).

[TotalVerb]

I'm cautiously optimistic that simply removing Base.@_pure_meta is OK, and inference can infer everything anyway. So far I haven't noticed any inference regressions and make test still passes. Could you/someone else run nanosoldier to make sure nothing regresses?

[JeffBezanson]

Why is eltype(a) not pure? Is anything more complex than f() = 1 pure? Or is that also not pure?

[ararslan]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[TotalVerb]

None of the regressions/improvements make any sense 😕. But a lot of the regressions are pretty substantial. (The improvements are probably noise.)

[ararslan]

Interestingly, the nullable benchmarks seem to be unaffected.

[TotalVerb]

The dot calls are probably not benchmarked. We should add benchmarks for them.

[ararslan]

If you mean broadcast for nullables, that appears to be the case: https://github.com/JuliaCI/BaseBenchmarks.jl/blob/master/src/nullable/NullableBenchmarks.jl

[jrevels]

Edit: Hm, maybe that's not the right way to invoke the nullable benchmarks...

It should be, but Nanosoldier won't catch it if you edit your submission after submitting it. Let's try with a fresh submission:

@nanosoldier runbenchmarks("nullable", vs=":master")

[nanosoldier]

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

[JeffBezanson]

Could you give an example of the codegen regression --- e.g. a particular code_typed or code_llvm that demonstrates the problem?

[TotalVerb]

@JeffBezanson Sure, see #21305. I'm currently bisecting to see if I can find the commit that changed this.

[JeffBezanson]

@TotalVerb Hopefully this PR is not needed any more --- can you double check?

[TotalVerb]

@JeffBezanson #21310 is a nice change but I don't think it's the particular problem here. The file from #21305 still has long LLVM:

eltypestuple() = Tuple{}
eltypestuple(x, xs...) = Tuple{eltype(x), eltypes(xs...).types...}

null_safe_op(::Any, ::Type, ::Type...) = false
Base.@pure null_safe_eltype_op(f, xs...) =
    null_safe_op(f, eltypestuple(xs...).types...)

@inline function broadcast_c(f, a...)
    if null_safe_eltype_op(f, a...)
        0
    else
        1
    end
end

f() = broadcast_c(1:10, 1:10, 1:10)

code_llvm(STDOUT, f, Tuple{})

The return value is correctly inferred as a constant, but the issue is that there is no way it seems for inference to figure out that null_safe_op is actually effect free and so the expensive call still takes place. (as a consequence of #20726)

[TotalVerb]

The @code_warntype demonstrates that the return value is inferred, but the call itself is still performed for side effects:

Variables:
  #self#::#f

Body:
  begin 
      $(Expr(:inbounds, false))
      # meta: location /tmp/regression.jl broadcast_c 9
      (Core._apply)(Main.null_safe_op, (Core.tuple)($(Expr(:new, UnitRange{Int64}, 1, :((Base.select_value)((Base.sle_int)(1, 10)::Bool, 10, (Base.sub_int)(1, 1)::Int64)::Int64))))::Tuple{UnitRange{Int64}}, (Core.getfield)((Core._apply)(Core.apply_type, (Core.tuple)(Main.Tuple, Int64)::Tuple{DataType,DataType}, (Core.getfield)((Main.eltypes)($(Expr(:new, UnitRange{Int64}, 1, :((Base.select_value)((Base.sle_int)(1, 10)::Bool, 10, (Base.sub_int)(1, 1)::Int64)::Int64))))::ANY, :types)::ANY)::ANY, :types)::ANY)::Bool
      goto 6 # line 10:
      6: 
      # meta: pop location
      $(Expr(:inbounds, :pop))
      return 1
  end::Int64

[JeffBezanson]

In that code, eltypes is not defined. It works if you use Base.eltypes.

[TotalVerb]

My bad! You're right, this problem has been fixed.