Switch from IRTools to CodeInfo #126

KDr2 · 2022-02-23T11:15:25Z

Fix #117

KDr2 · 2022-02-23T11:37:43Z

It turns out CodeInfo differs REALLY A LOT between versions...

yebai · 2022-02-23T11:44:21Z

It turns out CodeInfo differs REALLY A LOT between versions...

Let's focus on Julia >=1.7 in this PR.

KDr2 · 2022-02-23T11:46:07Z

Take function g below as an exmaple:

function g(x, y)
  if x>y
    r= string(sin(x))
  else
    r= sin(x) * cos(y)
  end
  return r
end

Its CodeInfo under 1.6:

 :(Base.slt_int(_3, _2))
 :(goto %7 if not %1)
 :(Base.sitofp(Float64, _2))
 :($(Expr(:invoke, MethodInstance for sin(::Float64), :(Base.Math.sin), :(%3))))
 :($(Expr(:invoke, MethodInstance for string(::Float64), :(Main.string), :(%4))))
 :(goto %12)
 :(Base.sitofp(Float64, _2))
 :($(Expr(:invoke, MethodInstance for sin(::Float64), :(Base.Math.sin), :(%7))))
 :(Base.sitofp(Float64, _3))
 :($(Expr(:invoke, MethodInstance for cos(::Float64), :(Base.Math.cos), :(%9))))
 :(Base.mul_float(%8, %10))
 :(φ (%6 => %5, %11 => %11))
 :(return %12)

Under 1.7

 Core.NewvarNode(:(_4))
 :(_2 > _3)
 :(goto %7 if not %2)
 :(Main.sin(_2))
 :(_4 = Main.string(%4))
 :(goto %10)
 :(Main.sin(_2))
 :(Main.cos(_3))
 :(_4 = %7 * %8)
 :(return _4)

Under 1.8:
CodeInfoToosl.jl throws en error while getting its CodeInfo, code fetched by @code_lwoered:

julia> @code_lowered g(1.,2.)
CodeInfo(
1 ─      Core.NewvarNode(:(r))
│   %2 = x > y
└──      goto #3 if not %2
2 ─ %4 = Main.sin(x)
│        r = Main.string(%4)
└──      goto #4
3 ─ %7 = Main.sin(x)
│   %8 = Main.cos(y)
└──      r = %7 * %8
4 ┄      return r
)

Conclusion:

CodeInfo changed a lot between v1.6 and v1.7
CodeInfoTools.jl doesn't work on v1.8

yebai · 2022-02-23T11:52:33Z

CodeInfo changed a lot between v1.6 and v1.7

Good to know, probably due to the major compiler refactoring in 1.7.

CodeInfoTools.jl doesn't work on v1.8

Maybe raise an issue on the CodeInfoTools package, or help fix it if not major?

KDr2 · 2022-02-23T11:53:11Z

Maybe we can remove the dependency of CodeInfoTools.jl too, all we want is to get the CodeInfo yet, but not to update the CodeInfo, so maybe there's a simple way to do that.

yebai · 2022-02-23T12:00:38Z

Maybe we can remove the dependency of CodeInfoTools.jl too, all we want is to get the CodeInfo yet, but not to update the CodeInfo, so maybe there's a simple way to do that.

We will likely need to update CodeInfo in the future, e.g. when we inline all the instructions. So we will probably build up a set of utilities for manipulating CodeInfo, which will be similar to CodeInfoTools. If that's the case, it might be better to depend on CodeInfoTools and help to improve it.

I took a quick look at the 1.8 error, and it appears that Julia has a simple function signature change between 1.7 and 1.8. It should be possible to fix that quickly.

yebai · 2022-02-23T12:02:58Z

CC @femtomc

Project.toml

src/tapedfunction.jl

femtomc · 2022-02-23T13:12:51Z

Re — open issue in CodeInfoTools with stacktrace for failure on > 1.8.

KDr2 · 2022-02-23T13:24:40Z

Re — open issue in CodeInfoTools with stacktrace for failure on > 1.8.

I opened 2 related issues:

KDr2 · 2022-02-23T14:54:52Z

All tests passed with JuliaCompilerPlugins/CodeInfoTools.jl#14.

femtomc · 2022-02-23T17:10:49Z

Re — inlining transforms should likely not live in CodeInfoTools. Ideally, the plugin (speculative) interface should be used to do this. It’s still WIP tho.

also, tracking types and things that are required to do inlining is possible in CodeInfoTools (just like IRTools) but there’s no really code to do that yet

rikhuijzer · 2022-02-24T06:29:18Z

The tests currently fail on the Turing test suite with the following errors (maybe more)

┌ Error: Unknown IR code:
│   typeof(line) = Core.SlotNumber
│   line = :(_28)
└ @ Libtask ~/git/Libtask.jl/src/tapedfunction.jl:291

and

dynamic model: Error During Test at /home/rik/git/Turing.jl/test/test_utils/staging.jl:42
  Got exception outside of a @test
  MethodError: no method matching iterate(::Nothing)
  Closest candidates are:
    iterate(::Union{LinRange, StepRangeLen}) at /nix/store/y46fn31jpgwkzj6hi1xjv0246x5w5dig-julia-bin-1.7.2/share/julia/base/range.jl:826
    iterate(::Union{LinRange, StepRangeLen}, ::Integer) at /nix/store/y46fn31jpgwkzj6hi1xjv0246x5w5dig-julia-bin-1.7.2/share/julia/base/range.jl:826
    iterate(::T) where T<:Union{Base.KeySet{<:Any, <:Dict}, Base.ValueIterator{<:Dict}} at /nix/store/y46fn31jpgwkzj6hi1xjv0246x5w5dig-julia-bin-1.7.2/share/julia/base/dict.jl:695
    ...
  Stacktrace:
    [1] consume(ttask::TapedTask{var"#imm#31"})
      @ Libtask ~/git/Libtask.jl/src/tapedtask.jl:144

KDr2 · 2022-02-24T15:27:45Z

The tests currently fail on the Turing test suite with the following errors (maybe more)

Hope 2e96940 fixes them.

PS: Why our IntegrationTest doesn't show the errors?

yebai · 2022-02-24T16:11:27Z

Many thanks, @femtomc would you like to make a release of CodeInfoTools so that tests can pass here?

devmotion · 2022-02-24T16:15:57Z

PS: Why our IntegrationTest doesn't show the errors?

Because Turing is not compatible with Libtask 0.7 or 0.8, and hence it is not possible to run them:
https://github.com/TuringLang/Libtask.jl/runs/5321568590?check_suite_focus=true#step:6:18

rikhuijzer · 2022-02-24T16:27:19Z

Because Turing is not compatible with Libtask 0.7 or 0.8, and hence it is not possible to run them:
https://github.com/TuringLang/Libtask.jl/runs/5321568590?check_suite_focus=true#step:6:18

We could add a bit of logic to the CI which removes the compat entry from Turing for these integration tests? Would require only a few lines of code. Ping me if I need to open a PR for this on this repo.

yebai · 2022-02-24T16:32:02Z

We could add a bit of logic to the CI which removes the compat entry from Turing for these integration tests? Would require only a few lines of code. Ping me if I need to open a PR for this on this repo.

Seconded - that would be nice! @rikhuijzer

devmotion · 2022-02-24T16:49:41Z

We could add a bit of logic to the CI which removes the compat entry from Turing for these integration tests? Would require only a few lines of code. Ping me if I need to open a PR for this on this repo.

I think we shouldn't do this. The point of breaking releases is to mark that there are breaking changes - so one can't expect downstream packages to work. And if downstream packages are broken it's not an issue since they explicitly state that they might not be compatible with this breaking release and also the package manager will never install them before the compat bound is updated.

devmotion · 2022-02-24T16:51:35Z

And if you are about to make a breaking release you can already now only bump the version in the final commit before the PR is merged - then the downstream packages will be tested and you will see test errors. In this PR the issue is that Turing is not even compatible with Libtask 0.7 though.

Project.toml

devmotion

I added some comments and suggestions. Overall it looks good, I can't comment too much on CodeInfoTools/IRTools specifics but I think it makes sense to move to the more stable and more lightweight alternative since it does not seem to increase complexity here.

Three more general comments:

We should remember to bump the version to 0.8 (or add it as the final commit in this PR once all tests, including Turing, pass)
We should try to add support for Julia 1.6 in a follow-up PR since it's the LTS version and supported by CodeInfoTools and all downstream packages
I think the extraction of the hardcoded isa statements in functions could be improved (in principle, it makes sense IMO): there is some code duplication and I think the Val argument should be removed (instead one could use branches in a (single!) function for Expr, possible calling other internal functions such as _expr_new...).

src/tapedfunction.jl

KDr2 · 2022-02-26T11:05:38Z

I think it's not a good idea to support Julia v1.6 with the new compiler facilities, it will be a great cost, I took a glance at the CodeInfo in v1.6, it's much more complicated (I'm not sure if this word is precise).

How about letting Libtask v0.7.x (an LTS version as well?) keep using IRTools.jl and letting Julia prior to v1.7 use that version.

rikhuijzer · 2022-02-26T14:48:39Z

How about letting Libtask v0.7.x (an LTS version as well?) keep using IRTools.jl and letting Julia prior to v1.7 use that version.

I think it’s a massive technical burden to maintain two versions in parallel

devmotion · 2022-02-26T15:31:43Z

Yes, and in particular versions that are so different. E.g., in SpecialFunctions we backport all bug fixes to SpecialFunctions 1 to ensure that they also reach packages that have not updated their compat entry (since SpecialFunctions is such a core package and so many other packages depend on it), and that's easy since the breaking change in SpecialFunctions 2.0.0 was only the removal of the type piracy factorial(::Number). However, if Libtask 0.7 depends on IRTools and 0.8 on CodeInfoTools, then I assume it will be much more difficult to backport bug fixes and import performance improvements.

devmotion · 2022-02-26T15:36:16Z

Is there a specific reason why it is difficult to support 1.6 but 1.8 works fine even though the CodeInfo is different from the one in 1.7 in both cases? In any case, I think it's fine to add support for 1.6 in a follow-up PR - but I also think it would be good to properly support, and hence also publish updates for, the LTS version.

yebai · 2022-02-26T21:16:59Z

Is there a specific reason why it is difficult to support 1.6 but 1.8 works fine even though the CodeInfo is different from the one in 1.7 in both cases?

There is a major compiler refactoring between 1.6 and 1.7. The CodeInfo data structures are quite similar for 1.7 and 1.8.

In any case, I think it's fine to add support for 1.6 in a follow-up PR - but I also think it would be good to properly support, and hence also publish updates for, the LTS version.

The old Libtask mechanism does support Julia 1.6 very well. The new tape-based mechanism is introduced for Julia 1.7. It is (partially) motivated by the high maintenance burden of Libtask for new Julia releases. It might not add too much value by backporting the new mechanism for previous Julia releases, since the old implementation works quite well.

devmotion

I think we should not even use Val for the Expr case. Unrelated of the Expr case, I think it is more natural to pass ir, taped, and boxes (not _box) as arguments to a function instead of constructing a callable. It seems there are also some undefined variables, so it seems not all functions are tested?

src/tapedfunction.jl

yebai

Thanks, @KDr2 - excellent work. I have some minor comments below, mostly about clarity.

In addition, the function names arg_boxer and args_initializer are a bit misleading. These two functions both refer to args but mean different things. The former refers to args for each instruction, while the latter refers to the args to a TapedFunction. Maybe we should make the distinction clearer.

src/tapedfunction.jl

KDr2 · 2022-02-27T00:06:56Z

In addition, the function names arg_boxer and args_initializer are a bit misleading.

I renamed arg_boxer to var_boxer.

KDr2 added 2 commits February 23, 2022 19:14

switch from IRTools to CodeInfo

5bb63dd

CodeInfoTools is only compatible with Julia >= 1.6

89185e1

devmotion reviewed Feb 23, 2022

View reviewed changes

Project.toml Show resolved Hide resolved

Project.toml Outdated Show resolved Hide resolved

src/tapedfunction.jl Outdated Show resolved Hide resolved

src/tapedfunction.jl Outdated Show resolved Hide resolved

src/tapedfunction.jl Outdated Show resolved Hide resolved

code refactor

a520829

KDr2 mentioned this pull request Feb 23, 2022

Remove the dependency on MacroTools #127

Closed

code refactor

ea9487d

fix goto position and some other bugs

2e96940

KDr2 added 2 commits February 25, 2022 00:01

optimize args_initializer

fe64428

minor update

be0a6df

yebai reviewed Feb 24, 2022

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

Update Project.toml

9eefb4a

KDr2 requested a review from devmotion February 26, 2022 04:36

devmotion reviewed Feb 26, 2022

View reviewed changes

code refactor

d4d6119

only dispatch Expr on Val(it.head)

2acb05f

devmotion reviewed Feb 26, 2022

View reviewed changes

yebai requested changes Feb 26, 2022

View reviewed changes

KDr2 added 4 commits February 27, 2022 08:07

code revise according to review comments

ef42c1c

don't dispatch on Val

ff19c96

return instruction from its builder

97d2cb4

put boxes into builder

45f792e

yebai force-pushed the master branch 9 times, most recently from 4a0af2f to a95702e Compare February 27, 2022 20:51

KDr2 added 2 commits March 1, 2022 23:16

code refactor

688df49

remove tape field from Instruction

a0586e1

yebai merged commit 0e00d76 into master Mar 2, 2022

delete-merged-branch bot deleted the ir-code-info branch March 2, 2022 21:39

yebai mentioned this pull request May 26, 2022

Widen Julia compat entry for 1.6 #147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from IRTools to CodeInfo #126

Switch from IRTools to CodeInfo #126

KDr2 commented Feb 23, 2022

KDr2 commented Feb 23, 2022

yebai commented Feb 23, 2022

KDr2 commented Feb 23, 2022

yebai commented Feb 23, 2022

KDr2 commented Feb 23, 2022

yebai commented Feb 23, 2022 •

edited

Loading

yebai commented Feb 23, 2022

femtomc commented Feb 23, 2022

KDr2 commented Feb 23, 2022 •

edited

Loading

KDr2 commented Feb 23, 2022

femtomc commented Feb 23, 2022

rikhuijzer commented Feb 24, 2022

KDr2 commented Feb 24, 2022 •

edited

Loading

yebai commented Feb 24, 2022

devmotion commented Feb 24, 2022

rikhuijzer commented Feb 24, 2022

yebai commented Feb 24, 2022

devmotion commented Feb 24, 2022

devmotion commented Feb 24, 2022

devmotion left a comment

KDr2 commented Feb 26, 2022

rikhuijzer commented Feb 26, 2022

devmotion commented Feb 26, 2022

devmotion commented Feb 26, 2022

yebai commented Feb 26, 2022

devmotion left a comment

yebai left a comment •

edited

Loading

KDr2 commented Feb 27, 2022

Switch from IRTools to CodeInfo #126

Switch from IRTools to CodeInfo #126

Conversation

KDr2 commented Feb 23, 2022

KDr2 commented Feb 23, 2022

yebai commented Feb 23, 2022

KDr2 commented Feb 23, 2022

yebai commented Feb 23, 2022

KDr2 commented Feb 23, 2022

yebai commented Feb 23, 2022 • edited Loading

yebai commented Feb 23, 2022

femtomc commented Feb 23, 2022

KDr2 commented Feb 23, 2022 • edited Loading

KDr2 commented Feb 23, 2022

femtomc commented Feb 23, 2022

rikhuijzer commented Feb 24, 2022

KDr2 commented Feb 24, 2022 • edited Loading

yebai commented Feb 24, 2022

devmotion commented Feb 24, 2022

rikhuijzer commented Feb 24, 2022

yebai commented Feb 24, 2022

devmotion commented Feb 24, 2022

devmotion commented Feb 24, 2022

devmotion left a comment

Choose a reason for hiding this comment

KDr2 commented Feb 26, 2022

rikhuijzer commented Feb 26, 2022

devmotion commented Feb 26, 2022

devmotion commented Feb 26, 2022

yebai commented Feb 26, 2022

devmotion left a comment

Choose a reason for hiding this comment

yebai left a comment • edited Loading

Choose a reason for hiding this comment

KDr2 commented Feb 27, 2022

yebai commented Feb 23, 2022 •

edited

Loading

KDr2 commented Feb 23, 2022 •

edited

Loading

KDr2 commented Feb 24, 2022 •

edited

Loading

yebai left a comment •

edited

Loading