WIP: Use Cassette.jl instead of Vinyl #11

pfitzseb · 2018-08-11T11:23:00Z

No description provided.

global pass seems to be working

maleadt · 2018-08-13T07:44:52Z

Some additional fixes, feel free to merge in this PR: sp/cassette...maleadt:pr/11

Pr/11

pfitzseb · 2018-08-30T09:21:23Z

Ok, so apart from the dispatch pass everything in the dynamic mode seems to be working. Couple of points:

The return type pass checks for "small unions" and doesn't warn if it encounters them. Not sure if that is a good thing to do.
I have no idea how (and why) the dynamic dispatch pass worked, so would appreciate some help in reviving that.
@MikeInnes Can you enable Travis for this repo?

MikeInnes · 2018-09-03T10:30:12Z

Maybe just check for Union{T,Missing/Nothing}? If people complain about other cases we can add something more general.
Dynamic dispatches are signalled by normal call expressions (as opposed to invoke) in code_typed. So it should be as simple as looking for those, unless I'm missing something
on it

MikeInnes · 2018-09-03T10:32:36Z

src/analysis.jl

  end
 end

 exprtype(code, x) = typeof(x)
-exprtype(code, x::Expr) = x.typ
+exprtype(code, x::Expr) = Union{}


I don't think this should be called any more, so perhaps we can just remove it. The same information should be in the SSAValue type.

True, I meant to remove that. There's a bit of cruft around anyways.

MikeInnes · 2018-09-03T10:33:30Z

src/analysis.jl


 rebuild(code, x) = x
 rebuild(code, x::QuoteNode) = x.value
-rebuild(code, x::Expr) = Expr(x.head, rebuild.(code, x.args)...)
-rebuild(code, x::SlotNumber) = code.slotnames[x.id]
+rebuild(code, x::Expr) = Expr(x.head, rebuild.(Ref(code), x.args))


Wouldn't this still need to be splatted?

Yeah, seems that way. I thought there was a Expr(head, args) constructor too.

MikeInnes · 2018-09-03T10:35:40Z

src/analysis.jl

-    (isexpr(ex, :(=)) && isexpr(ex.args[2], GlobalRef)) || return
-    ref = ex.args[2]
-    isconst(ref.mod, ref.name) ||
+    ex isa Expr || return


GlobalRefs can be top level as well, so this might be best expressed with prewalk.

Not sure what you mean?

As in, this will catch statements of the form %5 = foo(Base.pi) but not of the form %4 = Base.pi; %5 = foo(%4)

Hm, seems to work fine (-ish, no clue why it says x + Main.y instead of x + foo):

julia> function f(x) foo = y x+foo end f (generic function with 1 method) julia> @code_typed f(2) CodeInfo( 2 1 ─ %1 = Main.y::Any │ 3 │ %2 = (x + %1)::Any │ └── return %2 │ ) => Any julia> @trace f(2) ┌ Warning: uses global variable Main.y │ method = f(x) in Main at none:2 └ @ Traceur none:2 ┌ Warning: foo is assigned as Any │ method = f(x) in Main at none:2 └ @ Traceur none:2 ┌ Warning: dynamic dispatch to x + Main.y │ method = f(x) in Main at none:2 └ @ Traceur none:3 ┌ Warning: returns Any │ method = f(x) in Main at none:2 └ @ Traceur none:-1 4

Ah, that's because this pass actually runs on the unoptimised code_typed, not the SSA IR of @code_typed.

pfitzseb · 2018-09-03T12:56:07Z

Okay, so except for the prewalk thing which I'm not entirely sure how to handle I should have taken care of most of your points.

dispatch printing is pretty bad though:

julia> @trace naive_sum([1.0])
┌ Warning: s is assigned as Int64
│   method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390
└ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390
┌ Warning: s is assigned as Float64
│   method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390
└ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:392
┌ Warning: dynamic dispatch to (isa)(φ (%15 => 0, %51 => %32), Float64)
│   method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390
└ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:392
┌ Warning: dynamic dispatch to (isa)(φ (%15 => 0, %51 => %32), Int64)
│   method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390
└ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:392
┌ Warning: returns Union{Float64, Int64}
│   method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390
└ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:-1
1.0

because it operates on the optimized code_typed. Not sure how to get back to something sane from there.

MikeInnes · 2018-09-03T15:29:24Z

I expect that printing the function name and line is enough there.

…

On Mon, 3 Sep 2018 at 13:56, Sebastian Pfitzner ***@***.***> wrote: Okay, so except for the prewalk thing which I'm not entirely sure how to handle I should have taken care of most of your points. dispatch printing is pretty bad though: julia> @trace naive_sum([1.0]) ┌ Warning: s is assigned as Int64 │ method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390 └ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390 ┌ Warning: s is assigned as Float64 │ method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390 └ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:392 ┌ Warning: dynamic dispatch to (isa)(φ (%15 => 0, %51 => %32), Float64) │ method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390 └ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:392 ┌ Warning: dynamic dispatch to (isa)(φ (%15 => 0, %51 => %32), Int64) │ method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390 └ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:392 ┌ Warning: returns Union{Float64, Int64} │ method = naive_sum(xs) in Main at untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:390 └ @ Traceur untitled-3e45b0bd2d71ea51bb6331b5aaf260a2:-1 1.0 because it operates on the optimized code_typed. Not sure how to get back to something sane from there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIY9uOCNk8qQyEtHR8QuxiwNw_WwBkNks5uXScBgaJpZM4V5Il1> .

pfitzseb · 2018-09-03T15:39:23Z

Hm, yeah, but we don't even have the right function there:

julia> function naive_sum(xs)
         s = 0
         for x in xs
           s += x
         end
         return s
       end
naive_sum (generic function with 1 method)

julia> @code_typed naive_sum([1.0])
CodeInfo(
...   
  6 ┄─ %16 = φ (#5 => 0, #15 => %32)::Union{Float64, Int64}                                             │
  │    %17 = φ (#5 => %11, #15 => %46)::Float64                                                         │
  │    %18 = φ (#5 => %12, #15 => %47)::Int64                                                           │
4 │    %19 = (isa)(%16, Float64)::Bool                                                                  │
  └───       goto #8 if not %19                                                                         │
  7 ── %21 = π (%16, Float64)                                                                           │
  │    %22 = (Base.add_float)(%21, %17)::Float64                                                        │╻     +
  └───       goto #11                                                                                   │
  8 ── %24 = (isa)(%16, Int64)::Bool                                                                    │
  └───       goto #10 if not %24                                                                        │
  9 ── %26 = π (%16, Int64)                                                                             │
  │    %27 = (Base.sitofp)(Float64, %26)::Float64                                                       ││╻╷╷╷  promote
  │    %28 = (Base.add_float)(%27, %17)::Float64                                                        ││╻     +
  └───       goto #11                                                                                   │
...
) => Union{Float64, Int64}

julia> @trace naive_sum([1.0])
┌ Warning: s is assigned as Int64
│   method = naive_sum(xs) in Main at none:2
└ @ Traceur none:2
┌ Warning: s is assigned as Float64
│   method = naive_sum(xs) in Main at none:2
└ @ Traceur none:4
┌ Warning: dynamic dispatch to (isa)(φ (%15 => 0, %51 => %32), Float64)
│   method = naive_sum(xs) in Main at none:2
└ @ Traceur none:4
┌ Warning: dynamic dispatch to (isa)(φ (%15 => 0, %51 => %32), Int64)
│   method = naive_sum(xs) in Main at none:2
└ @ Traceur none:4
┌ Warning: returns Union{Float64, Int64}
│   method = naive_sum(xs) in Main at none:2
└ @ Traceur none:-1
1.0

The dynamic dispatch is actually happening in +, at least on the level of the user code.

MikeInnes · 2018-09-04T13:27:43Z

Well, there isn't any dynamic dispatch happening in that function, thanks to the new union splitting. I should have mentioned that Expr(:call, ... is used both for dynamic dispatches and built-ins/intrinsics like Base.isa and Base.add_int, which we can probably just ignore in general.

To actually get a dynamic dispatch for testing we probably need to use a global or Any container type.

pfitzseb · 2018-09-05T11:21:00Z

Ok, the code works fine then:

julia> using Traceur

julia> y = 2; f(x) = x+y
f (generic function with 1 method)

julia> @trace f(2)
┌ Warning: uses global variable Main.y
│   method = f(x) in Main at none:1
└ @ Traceur none:1
┌ Warning: dynamic dispatch to x + Main.y
│   method = f(x) in Main at none:1
└ @ Traceur none:1
┌ Warning: returns Any
│   method = f(x) in Main at none:1
└ @ Traceur none:-1
4

~~How can I figure out what intrinsics/built-ins I should ignore?~~ Nvm, I think I got something.

MikeInnes · 2018-09-05T11:32:19Z

f isa Union{Core.Builtin,Core.IntrinsicFunction} should do the trick.

pfitzseb · 2018-09-05T11:39:37Z

Yeah, I was confused because we had that exact check in there already (via isprimitive), but gated behind a f isa GlobalRef, which wasn't true for isa.

AFAICT everything works fine now.

MikeInnes · 2018-09-05T14:05:27Z

src/analysis.jl

-    end
+  for (i, l) in enumerate(code.code)
+    ind = code.codelocs[i]
+    ind = clamp(ind, 1, length(code.linetable))


Perhaps we should just set the line to nothing in this case, otherwise it could be misleading

Or -1, which would be in line with how Base handles that.

Yeah that would be fine, as long as we do a reasonable printing.

MikeInnes · 2018-09-05T14:18:27Z

src/analysis.jl

+    idx += 1
+    (isexpr(ex, :(=)) && isexpr(ex.args[1], Core.SlotNumber)) || return
+    typ = code.ssavaluetypes[idx]
+    if typ isa Core.Compiler.Const


I think Core.Compiler.widenconst would acheive the same thing here.

MikeInnes · 2018-09-05T14:21:59Z

src/analysis.jl

  end
 end

 # return type

+function issmallunion(t)
+  ts = Base.uniontypes(t)
+  length(ts) == 1 && isconcretetype(ts) && return true


isconcretetype(ts::Vector)?

isconcretetype(first(ts))... :)

MikeInnes · 2018-09-05T14:23:55Z

src/trace.jl

@@ -1,3 +1,44 @@
+# struct Trace


Are we planning to recover this? Seems best to leave a comment explaining why code is there, or delete.

MikeInnes · 2018-09-05T14:26:15Z

src/trace.jl

-  result = overdub(ctx, f, args...)
-  analyse((a...) -> ctx.warn(Warning(a...)), C)
-  return result
+function Cassette.prehook(ctx::TraceurCtx, f, args...)


Previously this was (effectively) a posthook; best to keep it that way unless there's a specific reason to change it.

What's the reasoning for that? I mean it's (probably) trivial to change, but why is posthook more appropriate here?

The reasoning is that it tends to give warnings in the "right order", in the sense that type inference issues tend to propagate up the stack rather than down. e.g. in f(x) = ...; g(x), f using a global variable won't affect the inferability of g, but g doing so will likely break f as well; so printing in post order gives you the root cause first followed by knock-on effects.

(The order to analysis passes is also designed to fit that rule, though it's obviously heuristic).

Ah, makes sense.

pfitzseb · 2018-09-05T16:41:51Z

Ok, I think I have resolved all of the above points.

MikeInnes · 2018-09-06T15:46:07Z

Ok this looks good. Awesome work @pfitzseb.

pfitzseb and others added 5 commits August 11, 2018 12:21

transistion to cassette.jl

aa007f2

global pass seems to be working

fix

e0ebc52

progress?

f9fea2b

Print to raw streams to avoid deadlocks.

7eea4d5

Fix rebuild of SSAvalue.

6d9f20c

Merge pull request #12 from maleadt/pr/11

63a8f2a

Pr/11

pfitzseb mentioned this pull request Aug 29, 2018

Julia 1.0 compatability #13

Closed

fix most passes

626d3d7

MikeInnes reviewed Sep 3, 2018

View reviewed changes

pfitzseb added 5 commits September 3, 2018 13:00

bit of cleanup

92f6f36

only ignore small unions with Missing/Nothing

f176f28

"fix" dispatch pass

f8505b4

fix tests. warn for Any return value

c824cb6

fix travis.yml

23c935f

fix dynamic dispatch pass

fdf1c01

MikeInnes reviewed Sep 5, 2018

View reviewed changes

cleanup

30ee36c

special case module access in global pass

50bb98a

MikeInnes merged commit bd94628 into master Sep 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Use Cassette.jl instead of Vinyl #11

WIP: Use Cassette.jl instead of Vinyl #11

pfitzseb commented Aug 11, 2018

maleadt commented Aug 13, 2018

pfitzseb commented Aug 30, 2018

MikeInnes commented Sep 3, 2018

MikeInnes Sep 3, 2018

pfitzseb Sep 3, 2018

MikeInnes Sep 3, 2018

pfitzseb Sep 3, 2018

MikeInnes Sep 3, 2018

pfitzseb Sep 3, 2018

MikeInnes Sep 3, 2018

pfitzseb Sep 5, 2018 •

edited

MikeInnes Sep 5, 2018

pfitzseb commented Sep 3, 2018

MikeInnes commented Sep 3, 2018 via email

pfitzseb commented Sep 3, 2018

MikeInnes commented Sep 4, 2018

pfitzseb commented Sep 5, 2018 •

edited

MikeInnes commented Sep 5, 2018

pfitzseb commented Sep 5, 2018

MikeInnes Sep 5, 2018

pfitzseb Sep 5, 2018

MikeInnes Sep 5, 2018

MikeInnes Sep 5, 2018

MikeInnes Sep 5, 2018

pfitzseb Sep 5, 2018

MikeInnes Sep 5, 2018

MikeInnes Sep 5, 2018

pfitzseb Sep 5, 2018

MikeInnes Sep 5, 2018

pfitzseb Sep 5, 2018

pfitzseb commented Sep 5, 2018

MikeInnes commented Sep 6, 2018

WIP: Use Cassette.jl instead of Vinyl #11

WIP: Use Cassette.jl instead of Vinyl #11

Conversation

pfitzseb commented Aug 11, 2018

maleadt commented Aug 13, 2018

pfitzseb commented Aug 30, 2018

MikeInnes commented Sep 3, 2018

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

pfitzseb Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfitzseb commented Sep 3, 2018

MikeInnes commented Sep 3, 2018 via email

pfitzseb commented Sep 3, 2018

MikeInnes commented Sep 4, 2018

pfitzseb commented Sep 5, 2018 • edited

MikeInnes commented Sep 5, 2018

pfitzseb commented Sep 5, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfitzseb commented Sep 5, 2018

MikeInnes commented Sep 6, 2018

pfitzseb Sep 5, 2018 •

edited

pfitzseb commented Sep 5, 2018 •

edited