fix #20704, `pure` annotation should not skip method errors #20726

JeffBezanson · 2017-02-21T21:02:57Z

There were two issues here:

We should not inline a call to a @pure function as a constant unless the inferred argument type is a subtype of the method signature, to be sure there is no MethodError.
Even if a function is marked @pure and is inferred to return a constant, we should not use the constant calling convention (CCC) for it unless we can prove it effect_free. That's because the CCC is equivalent to deleting the entire body of the function, which is not the same thing as calling the function at a different time.

Item (1) is an uncontroversial bug fix. Item (2) is more of an interesting corner case. I think most people would agree that if you put @pure on a function that prints, you shouldn't be surprised to see the printed output more or fewer times than you expected (including zero times). Similarly, consider

@pure function f()
    if isdir("/")
        error()
    end
    return 42
end

Here I'd even say it's ok to skip throwing the error, since who's to say whether isdir("/") is true in the imaginary land of pure functions. However I think method errors are different. We can only conclude that a function returns x if it actually calls a method that returns x. If the method that returns x is not called at all for a certain argument, it can't be the result even of a pure function.

vtjnash · 2017-02-21T21:12:20Z

base/inference.jl

+                # and to possibly enable more optimization in the future
+                me.const_api = true
+            end
+            if proven_pure || me.src.pure


We shouldn't inline me.src.pure functions, some of them can be very large

You're right, but we seem to have been doing that before! I guess I should just use proven_pure?

Ooops! and yes (merge with previous if statement)

It wasn't causing issues in the past since we also set me.const_api, so it should have managed to avoid actually inlining the whole function.

vtjnash · 2017-02-21T21:20:32Z

base/inference.jl

                        break
                    end
                end
            end
        end
-        me.src.pure = ispure
+        if proven_pure
+            me.src.pure = true


This doesn't seem quite right, but do we ever actually look at this field? I suspect we should acknowledge that source.pure and inferred.pure and reflecting slightly different attributes (the former meaning @pure, the latter meaning proven_pure). There's a bit of a mis-match in terminology here though too, since we're using me.src.pure to mean the same as const_api / jlcall_api == 2. However, it's perfectly reasonable to know that a function is effect-free / pure, but not know what it returns.

vtjnash · 2017-02-21T21:24:30Z

While I agree with this PR and think we should make both changes 1 and 2, I don't think this fixes #20704. To fix that issue, we need to fix the condition here (https://github.com/JuliaLang/julia/pull/20726/files#diff-c0d983631c937ee5acfd0137cd7332deR3784) to prove that we've called the function at least once.

JeffBezanson · 2017-02-21T21:27:53Z

I wanted to remove that block of code actually, but it broke a test in test/inference.jl that checks the behavior of a @pure function that calls rand(). I would rather say the behavior of such a function is undefined.

JeffBezanson · 2017-02-21T22:01:25Z

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

StefanKarpinski · 2017-02-21T22:12:56Z

I would rather say the behavior of such a function is undefined.

Wouldn't that contradict the idea that @pure is just a way of giving the compiler permission to call the function at any time?

JeffBezanson · 2017-02-21T23:07:18Z

Wouldn't that contradict the idea that @pure is just a way of giving the compiler permission to call the function at any time?

But when you call the function affects which random number you get. If you call it three times, will you get the same number or different ones? Depends on what the compiler decided to do, hence undefined.

JeffBezanson · 2017-02-21T23:14:30Z

I should clarify I only meant this in a very narrow sense related to the test in question --- the test was checking that repeated calls returned the same value, which should not be mandated. We can continue to call the function at run time if we want to.

nanosoldier · 2017-02-22T01:30:58Z

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

vtjnash · 2017-02-22T02:40:47Z

I wanted to remove that block of code actually

If we can get rid of that, we can just get rid of @pure entirely. At least half of the reason it exists is to ensure that we can assume the functions are effect_free, entirely skip the cost of calling the function, and inline the result as a constant.

The test with rand() is just making sure that this facet is working.

JeffBezanson · 2017-02-22T03:01:20Z

Yes, I see what you mean. I'll push a better version.

JeffBezanson · 2017-02-22T04:08:31Z

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

vtjnash · 2017-02-22T07:19:06Z

base/inference.jl


    methsig = method.sig
    if !(atype <: metharg)
        return invoke_NF(argexprs, e.typ, atypes, sv, atype_unlimited,
                         invoke_data)
    end

+    # check whether call can be inlined to just a quoted constant value
+    if isa(f, widenconst(ft)) && !method.isstaged && (method.source.pure || f === return_type) &&
+        are_args_const(atypes)


It's not sufficient to check that the result could have come from calling the pure function. We need to actually prove that it did come from calling the pure function. For example, we would be correct to infer that bar(1.0)::Const(1) in the original issue, even though calling bar(1.0) throws a method error.

Do we ever actually do that though? I believe any time these conditions are met, we will have called the function. I agree that's a bit brittle, but it's not easy to know where a type came from.

Here's a simple example of the failure scenario – it doesn't even need arguments :P – but it's just a minor rewrite of #20704:

julia> function method_error end method_error (generic function with 0 methods) julia> Base.@pure a() = (method_error(); 1) a (generic function with 1 method) julia> b() = a() b (generic function with 1 method) julia> b() 1

I think we either need to recompute the Const here, or add a bool field to const (was_computed_by_apply_pure) that we can check here.

nanosoldier · 2017-02-22T07:33:51Z

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

JeffBezanson · 2017-02-23T01:23:20Z

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

JeffBezanson · 2017-02-23T01:24:57Z

Ok, here's another attempt.

vtjnash · 2017-02-23T02:10:08Z

base/inference.jl

@@ -191,7 +193,7 @@ mutable struct InferenceState
                    vararg_type = rewrap(vararg_type, linfo.specTypes)
                end
                s_types[1][la] = VarState(vararg_type, false)
-                src.slottypes[la] = widenconst(vararg_type)
+                src.slottypes[la] = vararg_type


I think you can drop this part of the change. I don't foresee any need for inference or optimization to examine slottypes (it should be strictly suboptimal information relative to the dataflow-sensative types) – I think this is strictly consumed by codegen.

For slottypes in general that's true, but this was causing arguments to be given e.g. type Void instead of Const(nothing). Ideally we'd treat those the same anyway, but this was an easier way to achieve that.

Where are we using this info? Seems like this is a different bug that'll need tests if this is also causing issues.

The weird thing is that just a few lines above, we explicitly do

if isa(atyp, DataType) && isdefined(atyp, :instance) atyp = Const(atyp.instance)

and then call widenconst(atyp)! I'll try to find a test case.

I found it: exprtype reads slottypes, and inlining and some other passes call exprtype. It takes a really obscure case to trigger it. For example f(x) = x(), where f is inferred on the type of a singleton pure function. x() won't get inlined in that case because it's not a Const.

E.g.

Base.@pure function f(x) rand() 42 end julia> h(x) = x(nothing) h (generic function with 1 method) julia> code_typed(h, (typeof(f),)) 1-element Array{Any,1}: CodeInfo(:(begin return $(Expr(:invoke, MethodInstance for f(::Void), :(x), :(Main.nothing))) end))=>Int64

vtjnash · 2017-02-23T02:10:18Z

base/inference.jl

@@ -1569,7 +1571,7 @@ function pure_eval_call(f::ANY, argtypes::ANY, atype::ANY, vtypes::VarTable, sv:
    try
        value = Core._apply_pure(f, args)
        # TODO: add some sort of edge(s)
-        return abstract_eval_constant(value)
+        return Const(value,true)


missing space after the comma

vtjnash · 2017-02-23T02:14:21Z

base/inference.jl


    methsig = method.sig
    if !(atype <: metharg)
        return invoke_NF(argexprs, e.typ, atypes, sv, atype_unlimited,
                         invoke_data)
    end

+    # check whether call can be inlined to just a quoted constant value
+    if isa(f, widenconst(ft)) && !method.isstaged


Any idea why this isa check is here? AFAIK, we should be able to drop the f argument, but this check gave me pause, from uncertainty.

nanosoldier · 2017-02-23T04:43:17Z

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

vtjnash · 2017-02-27T22:11:55Z

Were all those regressions just noise then? It would have been nice to address those with at least a comment before merging.

tkelman · 2017-02-27T22:26:37Z

those look too large and consistent to be noise

JeffBezanson · 2017-02-27T22:37:45Z

Which set of results are you looking at?

vtjnash · 2017-02-28T00:05:13Z

The last one run (#20726 (comment)) had a max 2.38x slowdown and max 0.30x speedup. It seems neither was expected?

JeffBezanson · 2017-02-28T05:24:08Z

We've seen the sqrtm slowdown before --- did we ever figure out what it was?

I'll look at the IR for some of the vector operations.

vtjnash · 2017-02-28T06:59:17Z

daily benchmark did worse overall as well: (https://github.com/JuliaCI/BaseBenchmarkReports/blob/03d8b02711ecfc121c7935b5a20c66645fdd3f0a/daily_2017_2_28/report.md). Maybe this commit broke the inliner heuristics?

jrevels · 2017-02-28T13:29:24Z

JuliaCI/BaseBenchmarks.jl#65 is potentially relevant here.

JeffBezanson · 2017-02-28T20:35:47Z

Found something: code for _broadcast_eltype is no longer getting fully replaced with the resulting type. I'll try to fix it.

vtjnash · 2017-02-28T20:50:06Z

Ah, that makes sense. It looks like that one would be fairly hard for it to prove is pure, given our current lack of a framework for propagating the purity computation. How did it manage to get elided before?

Sacha0 · 2017-03-06T17:13:15Z

Ref. #20802 (comment) for what seems to be another related regression. Best!

improve `effect_free` to fix regressions from #20726

vtjnash reviewed Feb 21, 2017

View reviewed changes

JeffBezanson force-pushed the jb/fix20704 branch from 229e87e to 4f1f8f6 Compare February 21, 2017 22:00

JeffBezanson force-pushed the jb/fix20704 branch from 4f1f8f6 to a441a50 Compare February 22, 2017 04:08

vtjnash reviewed Feb 22, 2017

View reviewed changes

kshyatt added the error handling Handling of exceptions by Julia or the user label Feb 22, 2017

JeffBezanson force-pushed the jb/fix20704 branch from a441a50 to 6540021 Compare February 23, 2017 01:22

vtjnash approved these changes Feb 23, 2017

View reviewed changes

fix #20704, pure annotation should not skip method errors

8fee77f

JeffBezanson force-pushed the jb/fix20704 branch from 6540021 to 8fee77f Compare February 27, 2017 19:09

JeffBezanson merged commit d3de8cc into master Feb 27, 2017

vtjnash deleted the jb/fix20704 branch February 27, 2017 22:11

JeffBezanson added a commit that referenced this pull request Feb 28, 2017

improve effect_free to fix regressions from #20726

741ec6a

JeffBezanson added a commit that referenced this pull request Feb 28, 2017

improve effect_free to fix regressions from #20726

3475b6c

JeffBezanson added a commit that referenced this pull request Mar 1, 2017

improve effect_free to fix regressions from #20726

33dac60

Sacha0 mentioned this pull request Mar 6, 2017

Performance hit with broadcasting over tuples #20802

Closed

JeffBezanson added a commit that referenced this pull request Mar 6, 2017

improve effect_free to fix regressions from #20726

2f800a5

JeffBezanson added a commit that referenced this pull request Mar 7, 2017

Merge pull request #20843 from JuliaLang/jb/fix20726

2e12d10

improve `effect_free` to fix regressions from #20726

This was referenced Apr 6, 2017

Codegen change during 0.6 development cycle #21305

Closed

Refactor null_safe_op to workaround codegen changes #21290

Closed

fix #20704, pure annotation should not skip method errors #20726

fix #20704, pure annotation should not skip method errors #20726

Conversation

JeffBezanson commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Feb 21, 2017

JeffBezanson commented Feb 21, 2017

JeffBezanson commented Feb 21, 2017

StefanKarpinski commented Feb 21, 2017

JeffBezanson commented Feb 21, 2017

JeffBezanson commented Feb 21, 2017

nanosoldier commented Feb 22, 2017

vtjnash commented Feb 22, 2017

JeffBezanson commented Feb 22, 2017

JeffBezanson commented Feb 22, 2017

Choose a reason for hiding this comment

JeffBezanson Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Feb 22, 2017

JeffBezanson commented Feb 23, 2017

JeffBezanson commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Feb 23, 2017

vtjnash commented Feb 27, 2017

tkelman commented Feb 27, 2017 • edited Loading

JeffBezanson commented Feb 27, 2017

vtjnash commented Feb 28, 2017

JeffBezanson commented Feb 28, 2017

vtjnash commented Feb 28, 2017

jrevels commented Feb 28, 2017

JeffBezanson commented Feb 28, 2017

vtjnash commented Feb 28, 2017

Sacha0 commented Mar 6, 2017

fix #20704, `pure` annotation should not skip method errors #20726

fix #20704, `pure` annotation should not skip method errors #20726

JeffBezanson Feb 22, 2017 •

edited

Loading

tkelman commented Feb 27, 2017 •

edited

Loading