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

tidy up Slots in the compiler #48693

Merged
merged 3 commits into from
Feb 17, 2023
Merged

tidy up Slots in the compiler #48693

merged 3 commits into from
Feb 17, 2023

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Feb 16, 2023

This PR consists of two commits:

  • remove Slot type from the compiler: since only SlotNumber appears in lowering and TypedSlot only temporarily appears in a specific phase of inference, there is no need to define abstract type for SlotNumber and TypedSlot
  • move TypedSlot definition into Core.Compiler: only Julia-level compiler needs to deal with the type

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

closes #27202

@aviatesk aviatesk requested a review from vtjnash February 16, 2023 10:45
@@ -39,7 +39,7 @@ const TAGS = Any[
Float16, Float32, Float64, Char, DataType, Union, UnionAll, Core.TypeName, Tuple,
Array, Expr, LineNumberNode, :__LabelNode__, GotoNode, QuoteNode, CodeInfo, TypeVar,
Core.Box, Core.MethodInstance, Module, Task, String, SimpleVector, Method,
GlobalRef, SlotNumber, TypedSlot, NewvarNode, SSAValue,
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it looks like we need to replace this TypedSlot with something defined in Core?

Copy link
Member

Choose a reason for hiding this comment

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

We could just convert to a SlotNumber on load from this old tag id (ignore the typ), though I don't think anyone would be using this

Copy link
Member

Choose a reason for hiding this comment

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

I think you need to at least keep a placeholder here though to keep the numbering constant

Copy link
Member Author

Choose a reason for hiding this comment

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

Can Const for example be a placeholder here? Or is something other better?

@nanosoldier
Copy link
Collaborator

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

@aviatesk
Copy link
Member Author

closes #27202.

Since `TypedSlot` is now used as a very temporary object that only
appears in a very specific point of inference.
@giordano
Copy link
Contributor

closes #27202.

Leaving a comment like that does nothing, you probably want to add it to the first message of the PR, so that merging this PR will also automatically close the issue.

@aviatesk aviatesk merged commit c110f7c into master Feb 17, 2023
@aviatesk aviatesk deleted the avi/typedslot branch February 17, 2023 10:28
aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Feb 17, 2023
aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Feb 17, 2023
@maleadt
Copy link
Member

maleadt commented Feb 18, 2023

This change also broke Traceur and StableHashes. See the latest daily: https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_date/2023-02/17/report.html, lots of packages started failing today. IMO it would have been good to run PkgEval here and try to fix some packages before merging (it will take quite a while now before PkgEval recovers, given that we only test released versions).

davidanthoff added a commit to julia-vscode/TestItemRunner2.jl that referenced this pull request Jul 4, 2024
96ed09c version 0.9.32
0a8da3a minimum adjustments to make JuliaInterpreter work with v1.12 (#631)
397ea70 `public` as an identifier is deprecated (#612)
ce6e341 update builtins.jl (#628)
1024848 Merge pull request #629 from JuliaDebug/avi/54678
5700dcc adjustments to JuliaLang/julia#54428
ea522b8 adjustments to JuliaLang/julia#54678
eae3b4c Some more 1.12 compat (#625)
fc4aeca version 0.9.31
a265c14 [incomplete] changes needed to adapt to compressed line table format in Julia (#606)
31253a0 version 0.9.30
1b1b6d6 adjustments for Julia v1.11 (#615)
8043dbc add support for preset `current_scope` (#619)
55e33d0 use child `@testset` to get nicer test summary (#618)
e0e34be adapt to implicit leave change in Julia (#614)
f7138f9 v0.9.29
82b1552 Adjust to upcoming compiler change (#611)
011edf9 improve test code
78f766b minor improvements
713c768 implement support for `current_scope` (#605)
580b95c version 0.9.28
d7d4ced Fix revise#718 (#609)
0089e4b update docs and convert `jldoctest` to `@repl` blocks (#607)
d319168 Remove buggy linearization pass (#604)
1efae18 remove `AbstractFrameInstance` (#608)
0138e60 update CI.yml
6b1c476 version 0.9.27
ce20820 adjust to the `:enter` IR changes made in JuliaLang/julia#52300 (#599)
9afdf71 follow up #596 (#600)
9d50726 adapt to array changes in Julia base (#596)
68fa8be NFC: harden some internal ccalls (#595)
ccc1c95 remove old compats (#598)
15ad1c7 set CI timeout (#597)
7beca92 version 0.9.26
a0d0d33 Adjust to upcoming julia lowering change (#592)
a3cf18e Add a second link to the docs from the README (#589)
6da0b26 version 0.9.25
c8d1ef7 adjust to JuliaLang/julia#50943 (#585)
c93dedf adjust tests for latest Julia master (#584)
910cb6f Align arguments number in breakpoints hook docstring (#583)
7849d4a Bump actions/checkout from 2 to 3 (#576)
0169df2 Version 0.9.24
14e454b Ignore `:aliasscope` and `:popaliasscope` (#581)
3ab2674 Bump codecov/codecov-action from 1 to 3 (#577)
cc1bace Bump actions/cache from 1 to 3 (#578)
1d87867 Update some failing doctests (#579)
43f2041 enable dependabot for GitHub actions (#575)
aefaa30 use an explicit Any comprehension (#572)
475512b Version 0.9.23
8fecf35 Fix kw pattern matching, other changes on 1.9+ (#568)
cf7f437 also test on 1.9
57dbc98 version 0.9.22
d7a3dd4 fixup
b4d133d adjust to JuliaLang/julia#48693 (#566)
9026819 fix test on nightly (#564)
9c5454c Protect `error` calls from invalidation (#565)
da3fee2 remove unused import
2a1c076 bump version
6d2fbaf rejigger the code to compute the method instance in stacktraces (#563)

git-subtree-dir: packages/JuliaInterpreter
git-subtree-split: 96ed09c7127475d391b1a4f20906072f482278eb
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.

Make Core.Slot a Union type instead of a generic abstract type
5 participants