Performance regression on the threading branch compared to the threads branch

[kpamnany]

The reason the old threads branch is still around and hasn't been replaced by the newer threading branch is that there is a performance regression. There are three forms for the @threads macro:

1. @threads all foo()
2. @threads all begin ... end
3. @threads all for ... end

They all use the same threading call--the latter two generate an inner function to pass in--and should have the same performance. In the threads branch they do; in the threading branch, the latter two are ~3.5X slower than the first form.

Any of the three forms can be used in test/perf/threads/laplace3d.jl; choose the one to try at line 95.

@JeffBezanson: I think you had fixed this or something very similar to this in the threads branch back in November--something to do with type checks in inner functions?

[JeffBezanson]

I have this one by the tail. It comes down to how the array assignment gets inlined. In the fast version:

        (top(arrayset))(u3::Array{Float32,3},#s36::Float32,#56#k_1::Int64,k_2::Int64,k_3::Int64)::Array{Float32,3}

and in the slow version:

        _var13 = u3::Array{Float32,3}
        _var12 = #s36::Float32
        _var11 = #56#k_1::Int64
        _var10 = k_2::Int64
        _var9 = k_3::Int64
        (top(arrayset))(_var13::Array{Float32,3},_var12::Float32,_var11::Int64,_var10::Int64,_var9::Int64)::Array{Float32,3}

Just extra (though unnecessary) temp vars. Surprisingly, this causes a cascade of horrible effects in code gen. u3 comes from the closure environment. The extra temp var moves it to a GC root, which I believe throws off alias analysis (the closed vars are tbaa_const). Then we/llvm fail to hoist array metadata access out of the loop, and consequently also fail to vectorize the loop.

cc @vtjnash @Keno

[vtjnash]

It looks like inlining may be concerned about the evaluation order. I've seen that be an issue with some intrinsics which want to swap their argument order.

Since u3 is not local, it's usage is not affect-free. (I don't think that's related here though)

[JeffBezanson]

But u3 is never assigned, so it should be affect-free.

[vtjnash]

True. Plus it wouldn't explain why the Other vars are being extracted. More codegen passes now generate GenSym objects. Which still generate a symbol?

[JeffBezanson]

Somewhat related: #6713

[JeffBezanson]

Inlining still uses symbols instead of GenSyms, based on the islocal variable in inlineable. I don't think the logic around this variable is optimal. Whether the variable is assigned to seems to be what matters --- do I have that right?

[JeffBezanson]

I can fix this by making effect_free return true for more symbols. Surely the logic there now is way too conservative! Can a non-captured local variable ever change value while evaluating an argument list?

[vtjnash]

Inlining still uses symbols instead of GenSyms, based on the islocal variable in inlineable. I don't think the logic around this variable is optimal. Whether the variable is assigned to seems to be what matters --- do I have that right?

yes. if the symbol is local, that means the argument is also assigned locally. it's only valid to use a GenSym when the variable is assigned exactly once.

Can a non-captured local variable ever change value while evaluating an argument list?

can you give an example of what you mean? i thought a local variable would already always return true

[vtjnash]

insanity case?

function f() 
  x = 1
  g() = (x+=1)
  h(a,b,c,d) = (a,b,c,d)
  return h(x, g(), g(), x)
end

but the inliner would bail on that just due to the presence of the captured variable (

julia/base/inference.jl

Lines 2374 to 2377 in b201312

	if (vi[3]&1)!=0
	# captures variables (TODO)
	return NF
	end

)

[JeffBezanson]

Yeah, that's why I said "non-captured".
effect_free says:

    if isa(e,SymbolNode)
        allow_volatile && return true
        if is_global(sv, (e::SymbolNode).name)
            return false
        end
    end

In other words, it only returns true for a local when allow_volatile is true. Quite sub-optimal.

[vtjnash]

Ah, right. That's not falling through correctly and is being wayyyy too conservative

[JeffBezanson]

Ok I fixed this on master, and it will fix this issue when picked onto the threading branch.