Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rework on src.slottypes and ir.argtypes #49113

Merged
merged 2 commits into from
Mar 24, 2023
Merged

rework on src.slottypes and ir.argtypes #49113

merged 2 commits into from
Mar 24, 2023

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Mar 23, 2023

This commit reworks our handling of (src::CodeInfo).slottypes and (ir::IRCode).argtypes. Currently ir.argtypes contains types for call arguments and local slots even after the SSA conversion (slot2reg), which can be quite confusing. Similarly, src.slot[names|flags|types] contains information about local slots regardless of whether src is optimized or not, even though local slots never appear within src.

With this commit, we resize ir.argtypes so that it only contains information about call arguments after slot2reg, as well as resize src.slot[names|flags|types] after optimization.

This commit removes unnecessary information upon appropriate timings and allows us to use Core.OpaqueClosure(ir::IRCode) constructor for such ir that is retrieved by Base.code_ircode:

let ir = first(only(Base.code_ircode(sin, (Int,))))
    @test Core.OpaqueClosure(ir)(42) == sin(42)
    ir = first(only(Base.code_ircode(sin, (Float64,))))
    @test Core.OpaqueClosure(ir)(42.) == sin(42.)
end

@aviatesk aviatesk requested a review from Keno March 23, 2023 06:19
@aviatesk
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

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

@aviatesk
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

This commit reworks our handling of `(src::CodeInfo).slottypes` and
`(ir::IRCode).argtypes`. Currently `ir.argtypes` contains types for call
arguments and local slots even after the SSA conversion (`slot2reg`),
which can be quite confusing. Similarly, `src.slot[names|flags|types]`
contains information about local slots regardless of whether `src` is
optimized or not, even though local slots never appear within `src`.

With this commit, we resize `ir.argtypes` so that it only contains
information about call arguments after `slot2reg`, as well as resize
`src.slot[names|flags|types]` after optimization.

This commit removes unnecessary information upon appropriate timings and
allows us to use `Core.OpaqueClosure(ir::IRCode)` constructor for such
`ir` that is retrieved by `Base.code_ircode`:

```julia
let ir = first(only(Base.code_ircode(sin, (Int,))))
    @test OpaqueClosure(ir)(42) == sin(42)
    ir = first(only(Base.code_ircode(sin, (Float64,))))
    @test OpaqueClosure(ir)(42.) == sin(42.)
end
```
Comment on lines +61 to +62
# XXX the length of `ci.slotflags` may be different from the actual number of call
# arguments, but we really don't know that information in this case
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a new problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of. Previously we just created ir.argtypes based on the length of ci.slotflags no matter if ci.slotflags contains flags for local variables. That wasn't a problem, because the optimization passes don't care if ir.argtypes contains extra type information (and this is why we have used ci.slottypes as is for ir.argtypes in the main optimization pipeline).

After this commit, this code becomes a bit problematic since we will make additional efforts to make ir.argtypes contain types of call arguments only. Having said that, it's not that problematic usually (because the optimization pipeline doesn't care it currently). It would be problematic if we generate OpaqueClosure with ir created by inflate_ir(ci) with un-inferred ci, which triggers this code path, but it's such a rare case anyway.

base/compiler/optimize.jl Show resolved Hide resolved
base/opaque_closure.jl Outdated Show resolved Hide resolved
@aviatesk aviatesk merged commit 97cf96a into master Mar 24, 2023
@aviatesk aviatesk deleted the avi/ssa-slots branch March 24, 2023 03:57
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Mar 24, 2023
After JuliaLang/julia#49113, `src.slot[names|types|flags]` may be
resized by the optimization pipeline, so we have to make sure to store
our own copies of them to keep the pre-optimization state of `CodeInfo`.
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Mar 24, 2023
* adjust to JuliaLang/julia#49071

* adjust to JuliaLang/julia#49113

  After JuliaLang/julia#49113, `src.slot[names|types|flags]` may be
  resized by the optimization pipeline, so we have to make sure to store
  our own copies of them to keep the pre-optimization state of `CodeInfo`.
aviatesk added a commit to aviatesk/Diffractor.jl that referenced this pull request Mar 28, 2023
oxinabox pushed a commit to JuliaDiff/Diffractor.jl that referenced this pull request Mar 28, 2023
aviatesk added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Mar 28, 2023
aviatesk added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Mar 28, 2023
aviatesk added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Mar 28, 2023
aviatesk added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Mar 28, 2023
aviatesk added a commit to JuliaDiff/Diffractor.jl that referenced this pull request Mar 28, 2023
Keno added a commit that referenced this pull request Apr 15, 2023
This was changed in #49113 and is
usually not a problem, but Cthulhu captures the post-inference but unoptimized
CodeInfo, which was invalidated by the later modification of the slottypes
array. Fix this by copying the slottypes for IRCode construction instead.
Keno added a commit that referenced this pull request Apr 15, 2023
This was changed in #49113 and is
usually not a problem, but Cthulhu captures the post-inference but unoptimized
CodeInfo, which was invalidated by the later modification of the slottypes
array. Fix this by copying the slottypes for IRCode construction instead.
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
This commit reworks our handling of `(src::CodeInfo).slottypes` and
`(ir::IRCode).argtypes`. Currently `ir.argtypes` contains types for call
arguments and local slots even after the SSA conversion (`slot2reg`),
which can be quite confusing. Similarly, `src.slot[names|flags|types]`
contains information about local slots regardless of whether `src` is
optimized or not, even though local slots never appear within `src`.

With this commit, we resize `ir.argtypes` so that it only contains
information about call arguments after `slot2reg`, as well as resize
`src.slot[names|flags|types]` after optimization.

This commit removes unnecessary information upon appropriate timings and
allows us to use `Core.OpaqueClosure(ir::IRCode)` constructor for such
`ir` that is retrieved by `Base.code_ircode`:

```julia
let ir = first(only(Base.code_ircode(sin, (Int,))))
    @test OpaqueClosure(ir)(42) == sin(42)
    ir = first(only(Base.code_ircode(sin, (Float64,))))
    @test OpaqueClosure(ir)(42.) == sin(42.)
end
```

Co-authored-by: Jameson Nash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants