Issue #185

[mhauru]

This fixes #185, and adds a helpful error message if one tries to call TapedTask(_, produce, x) (which I suspect we can't make work for somewhat deep reasons).

The problem in #185 is that sometimes the IR has :invoke expressions where the thing being invoked isn't literally produce, but rather a IR/BB ID referencing a variable that is produce. In these cases is_produce_stmt fails to recognise that this is indeed a call to produce. At least in the cases that my MWE brings up, however, the :invoke still carries with it the knowledge that this is MethodInstance of produce, so I changed the check to look for that.

I don't know what I'm doing when I'm messing with things like MethodInstances, so would be very good to get @willtebbutt's eyes on this. Maybe there's a more robust way to fix this?

Closes #185

[github-actions]

Libtask.jl documentation for PR #187 is available at:
https://TuringLang.github.io/Libtask.jl/previews/PR187/

[sunxd3]

make sense to me, no comments here

[willtebbutt]

Hmm I'm a little confused as to why the fix fixes the problem -- it looks to me like the IRCode associated to g doesn't contain any invoke expressions which contain a call to produce (due to the type instability, you wind up with a :call expression being associated to the call to produce), so I'm surprised that the test involving g is sensitive to the change to the implementation of is_produce_stmt. Am I missing something?

[mhauru]

@willtebbutt, does this help?

julia> function g(x)
                   if x > 0
                       x = 2
                   else
                       x = 0.1
                   end
                   return produce(x)
               end
g (generic function with 2 methods)

julia> Base.code_ircode(g, (Float64,))
1-element Vector{Any}:
2 1 ─ %1  = Base.lt_float(0.0, _2)::Bool                              │╻╷╷ >
  │   %2  = Base.or_int(%1, false)::Bool                              ││╻   <
  └──       goto #3 if not %2                                         │
  2 ─       goto #4                                                   │
  3 ─       nothing::Nothing                                          │
7 4 ┄ %6  = φ (#2 => false, #3 => true)::Bool                         │
  │   %7  = φ (#2 => 2, #3 => 0.1)::Union{Float64, Int64}             │
  │   %8  = Main.produce::Core.Const(produce)                         │
  └──       goto #6 if not %6                                         │
  5 ─ %10 = π (%7, Float64)                                           │
  │   %11 = invoke %8(%10::Float64)::Union{Float64, Int64}            │
  └──       goto #8                                                   │
  6 ─       nothing::Nothing                                          │
  7 ─ %14 = π (%7, Int64)                                             │
  │   %15 = invoke %8(%14::Int64)::Union{Float64, Int64}              │
  └──       goto #8                                                   │
  8 ┄ %17 = φ (#5 => %11, #7 => %15)::Union{Float64, Int64}           │
  └──       return %17                                                │
   => Union{Float64, Int64}

At first %8 = Main.produce::Core.Const(produce) and then later %11 = invoke %8(%10::Float64)::Union{Float64, Int64} and %15 = invoke %8(%14::Int64)::Union{Float64, Int64}. Thus is_produce_stmt sees arguments like

x = :($(Expr(:invoke, MethodInstance for produce(::Int64), :(Main.produce), Libtask.BasicBlockCode.ID(0))))
x = :($(Expr(:invoke, MethodInstance for produce(::Int64), :(Main.produce), Libtask.BasicBlockCode.ID(2))))

which the old check didn't catch. Thus the produce got treated like any other call for which stmt_might_produce is false, and the test I put in failed with Evaluated: nothing == 5.

[willtebbutt]

Hmmm which version of Julia are you producing this IR from? I see the following on 1.11.5:

julia> Base.code_ircode(g, (Float64,))
1-element Vector{Any}:
2 1 ─ %1 = Base.lt_float(0.0, _2)::Bool                               │╻╷╷ >
  │   %2 = Base.or_int(%1, false)::Bool                               ││╻   <
  └──      goto #3 if not %2                                          │
  2 ─      goto #4                                                    │
  3 ─      nothing::Nothing                                           │
7 4 ┄ %6 = φ (#2 => 2, #3 => 0.1)::Union{Float64, Int64}              │
  │   %7 = Main.produce::Any                                          │
  │   %8 = (%7)(%6)::Any                                              │
  └──      return %8                                                  │
   => Any

and a similar thing on 1.10.9. It does look like you've got two methods of g your snippet though, whereas I'm running in a clean Julia session. Maybe that's where the difference comes from?

[mhauru]

Oooh, nice catch, I had a long-living environment. It scares me that having an unrelated method defined affects the IR code of this method 😬 . Anyway, I reverted back to using the MWE from the original issue, and checked that without the fix in this PR the test fails in a fresh session.

[willtebbutt]

Excellent. In any case, I was being really silly -- I hadn't installed Libtask in my environment or using Libtask: produce 🤦 , so I was getting irrelevant IR.

This looks perfect to me, so I'm happy for you to bump the patch version and tag a release.

[willtebbutt]

This is probably the right thing to do. In versions 1.10 and 1.11 it's definitely the case that the first argument in an :invoke expression must be a Core.MethodInstance (to the best of my knowledge). However, this might change in later versions, so this check feels appropriate.

[mhauru]

I've bumped version and merged. I'm trying to fix #186, might bundle the fix together in the same release, unless you prefer otherwise.

[willtebbutt]

No strong view :) Also not really my call though haha.