Let tmerge form a Union more often

[martinholters]

This addresses the performance regression of mapslices introduced by #27417 and noticed in #27828. I think replacing isconcretetype(typea) || isType(typea) with unioncomplexity(typea) == 0 makes a lot of sense and is in the same spirit as the original PR.

However, I had to introduce another condition due to this quirk (which happens both before #27417 and with this PR):

julia> foo(a,b,c,d) = SubArray(a,b,c,d)
foo (generic function with 1 method)

julia> atypes = Tuple{Union{IndexCartesian, IndexLinear}, Any, Union{Tuple{}, Tuple{Any,Vararg{Any,N} where N}}, Tuple}
Tuple{Union{IndexCartesian, IndexLinear},Any,Union{Tuple{}, Tuple{Any,Vararg{Any,N} where N}},Tuple}

julia> atypes2 = Tuple{Union{IndexCartesian, IndexLinear}, Any, Tuple, Tuple}
Tuple{Union{IndexCartesian, IndexLinear},Any,Tuple,Tuple}

julia> atypes <: atypes2
true

julia> code_typed(foo, atypes)
1-element Array{Any,1}:
 CodeInfo(
1 1 ─ %1 = Main.SubArray(%%a, %%b, %%c, %%d)::SubArray                      │
  └──      return %1                                                        │
) => SubArray

julia> code_typed(foo, atypes2)
1-element Array{Any,1}:
 CodeInfo(
1 1 ─ %1 = Main.SubArray(%%a, %%b, %%c, %%d)::Union{SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1, SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1}
  └──      return %1                                                        │
) => Union{SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1, SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1}

Note that the wider atypes2 gives the narrower inference result. Hence the additional Tuple condition to turn Union{Tuple{}, Tuple{Any,Vararg{Any,N} where N}} into Tuple. (They should probably be type-equal anyway, but that's a different issue.) This leaves me a bit unsatisfied, but seems to be necessary to fix the regression.

[JeffBezanson]

A non-monotonicity is pretty bad; I think it would be better to fix that and address the regression by improving the inlining logic (which I'm working on now).

[vtjnash]

Monotonicity is certainly very nice, but I assume v0.6 would have shown the same behavior? It’s only recently that I tried to push hard on these edge cases - they’re definitely very difficult to design.

[vtjnash]

I think an alternate literal translation of this would be isa(unwrap_unionall(T), DataType)

[martinholters]

I've looked a little bit into the non-montonicity. Union-splitting the atypes case gives these four combinations:

argtypes	return type
Tuple{IndexCartesian,Any,Tuple{},Tuple}	SubArray{_1,_2,_3,Tuple{},false} where _3 where _2 where _1
Tuple{IndexLinear,Any,Tuple{},Tuple}	SubArray{_1,_2,_3,Tuple{},true} where _3 where _2 where _1
Tuple{IndexCartesian,Any,Tuple{Any,Vararg{Any,N} where N},Tuple}	SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1
Tuple{IndexLinear,Any,Tuple{Any,Vararg{Any,N} where N},Tuple}	SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1

Then tmerge'ing the first two gives a Union and then tmerge'ing the third widens to SubArray. On the other hand, with atypes2, union-splitting only gives two combinations:

argtypes	return type
Tuple{IndexCartesian,Any,Tuple,Tuple}	SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1
Tuple{IndexLinear,Any,Tuple,Tuple}	SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1

These are tmerged into the Union, done.

Now interestingly, if the order of the tmerge operations in the first case was changed, one could obtain the same results, as the first return type is a subtype of the third and the second one is a subtype of the fourth. I have an idea how to make tmerge also give a Union in this case. I'll give it a try. Meanwhile, #27857 might be the better fix to address the issue that brought me here.

[martinholters]

Ok, this is now much more elegant. However, as the original issue is better tackled with #27857 (addressing the underlying problem instead of working around it), the need for this is much lower. It may still give slightly better inference results, but whether they give tangible benefits is another question. It does fix above-mentioned quirk, though, as now

julia> code_typed(foo, atypes)
1-element Array{Any,1}:
 CodeInfo(
1 1 ─ %1 = Main.SubArray(%%a, %%b, %%c, %%d)::Union{SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1, SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1}
  └──      return %1                                                        │
) => Union{SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1, SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1}

julia> code_typed(foo, atypes2)
1-element Array{Any,1}:
 CodeInfo(
1 1 ─ %1 = Main.SubArray(%%a, %%b, %%c, %%d)::Union{SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1, SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1}
  └──      return %1                                                        │
) => Union{SubArray{_1,_2,_3,_4,false} where _4 where _3 where _2 where _1, SubArray{_1,_2,_3,_4,true} where _4 where _3 where _2 where _1}

[martinholters]

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

[nanosoldier]

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

[martinholters]

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

[martinholters]

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

[martinholters]

@ararslan is the soldier down? I wanted to check how the recent changes, in particular wrt inlining, interact with this PR.

[ararslan]

Yeah there was a server error so I restarted it.

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

[nanosoldier]

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

[martinholters]

Ok, this doesn't make much of a difference performance-wise now. Mostly that's a hint that the inlining heuristics are saner now. I still find this change esthetically pleasing, but there is little to no tangible benefit, so I leave it to others to make the call here.

[StefanKarpinski]

It does seem nicer to me as well in my very mildly informed opinion. @JeffBezanson?

[JeffBezanson]

I would worry about compiler performance here; using more Union types will cause us to infer more signatures.

[vtjnash]

I think this seems good

[JeffBezanson]

If we're going to merge this it would be good to add some sort of test related to #27843 (comment) (the example from the OP basically).

[martinholters]

Not sure how good a benchmark of compiler performance building the sysmimg is, but:
master:

Total ─────── 140.776988 seconds 
Base: ───────  32.209240 seconds 22.8796%
Stdlibs: ────  92.753201 seconds 65.8866%
Precompile: ─  15.812288 seconds 11.2322%

this PR:

Total ─────── 147.342998 seconds 
Base: ───────  34.227599 seconds 23.2299%
Stdlibs: ────  97.176708 seconds 65.9527%
Precompile: ─  15.936632 seconds 10.816%

So, yes, might cost us a few percent.

Test to the same effect as OP, but not relying on the specifics of the SubArray constructor added.

[vtjnash]

They both give SubArray now, but apparently I said this is good, if you care to rebase

[martinholters]

Rebased. Let's see whether CI still likes it.

[martinholters]

Added a test to address #27843 (comment). CI had been all green, now there is a fail in SHA on tester_linux64 which I don't understand but assume unrelated. If someone can confirm it's unrelated to this PR, I'll go ahead and merge unless anyone objects.

[vtjnash]

It is an AMD cpu bug