slot2ssa: remove `TypedSlot` #50943

topolarity · 2023-08-16T18:40:22Z

Dependent on #50924 .

This removes the last remaining usage of TypedSlot.

As a bonus, this evicts all of the "tmerge" type-iteration that was being done in slot2ssa.

TODO:

use bb_vartables for SLOT_USEDUNDEF, so that we can remove find_undefs!
confirm perf improvement with benchmarks

topolarity · 2023-08-17T21:36:32Z

base/compiler/ssair/slot2ssa.jl

@@ -1,13 +1,5 @@
 # This file is a part of Julia. License is MIT: https://julialang.org/license

-struct TypedSlot


Worth mentioning that deleting this definition breaks JuliaInterpreter (and therefore Revise)

We'll put in a version check.

topolarity · 2023-08-18T04:31:27Z

base/compiler/typeinfer.jl

-    # finish marking used-undef variables
-    for j = 1:nslots
-        if undefs[j]
-            slotflags[j] |= SLOT_USEDUNDEF | SLOT_STATICUNDEF


The optimizer no longer relies on these flags, but will external consumers need them?

We'll find out. I'm somewhat sceptical, but if they are, let's wean them off it.

topolarity · 2023-08-18T04:33:41Z

I still need to fix up the test suite, but otherwise this is ready for review now.

test/compiler/inference.jl

This is the last usage of TypedSlot (we should be clear now to remove that step from the end of type inference now). Compared to the prior scheme, this now inserts PiNodes at every PhiNode corresponding to a given slot, rather than at each load. In principle, that should mean that the types we compute across PhiNodes are more precise than before. I believe that means that all slot uses should be at least as precise as before, but I haven't completely convinced myself yet.

Since this is now unused by `slot2ssa` downstream, we can happily remove it from the rest of the compiler. There's one more piece of logic that I still need to move downstream: we should use the `bb_vartables` for undefined-ness as well. That will allow us to finally remove the (now incorrectly named) `annotate_slot_load!`

Now that we have the `bb_vartables` from Inference, we can get a converged (and frequently more precise) result for our inserted PhiNodes directly, instead of trying to compute the fix point in the optimizer. This change also improves the way that we are inserting PiNodes to ensure that they are present in every block that requires them for precision, not just where we are already inserting PhiNodes. This is necessary due to the presence of `Conditional`, which means we may have a refined type in a block even when dominated by a definition whose type was precise outside of the Conditional.

Updates slot2ssa to use the `bb_vartables` for `undef` information. As a bonus, evicts the final usage of `tmerge` in slot2ssa. This also fixes a few crashes from the prior commit, esp. where I was emitting PiNode(UndefToken(), Union{}) or where the PhiC nodes were not being ordered correctly.

This allows downstream consumers, like the optimizer, to be sure that they are only visiting blocks that Inference also visited. Also update tests and remove some unnused variables in `slot2ssa`.

Now that type inference modifies branches to reflect static control flow information, we can't search for the branch that we want to optimize away.

Pangoraw

Minor comments from reading the PR.

For the slotflags validity, there is already SLOT_ASSIGNEDONCE and SLOT_USED ~~whose value seems to never be set (from my research in the codebase).~~

julia/base/compiler/optimize.jl

Line 12 in fdd16bf

const SLOT_ASSIGNEDONCE = 16 # slot is assigned to only once

julia/base/reflection.jl

Line 1244 in fdd16bf

const SLOT_USED = 0x8

base/compiler/ssair/slot2ssa.jl

Keno · 2023-08-18T21:45:53Z

For the slotflags validity, there is already SLOT_ASSIGNEDONCE and SLOT_USED whose value seems to never be set (from my research in the codebase).

Some of the slotflags are computed in the frontend

base/compiler/typeinfer.jl

base/show.jl

test/compiler/inference.jl

base/compiler/ssair/slot2ssa.jl

base/compiler/optimize.jl

base/compiler/typeinfer.jl

base/compiler/ssair/slot2ssa.jl

aviatesk

Looks great. There are few potential improvements like cleaning up type_annotate! further by shifting the optimization of GotoIfNot to slot2reg, but the changes are very nice overall.

This means that inference modifies types and not IR statements themselves. In order to make this happen, we pass have to pass per-statement reachability information to the optimizer. Also fixes up assorted review feedback.

We were doing this before, but the previous commit had changed it. The problem is that the optimizer cannot rely on the `unreachable` vector for its external entrypoint. This restores the existing logic so that type inference will give you a result that you can pipe into the optimizer and have it work, although suboptimally since we won't be able to prune any unreachable branches.

aviatesk

We need to update OptimizationState calls within our tests since the OptimizationState signature has been updated. Alternatively we can make it optional argument unreachable::BitSet=BitSet().

base/compiler/optimize.jl

I think it'd be a more maintainable data flow to have these results collected manually and explicitly passed to the conversion routine for the optimizer, but for now this follows the established pattern and allows us to create a complete OptimizationState from an InferenceState.

aviatesk · 2023-08-25T04:53:08Z

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

nanosoldier · 2023-08-25T05:46:05Z

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

aviatesk · 2023-08-25T06:31:58Z

The benchmark looks great now. Let's merge.

aviatesk · 2023-08-25T06:33:16Z

Great job @topolarity!

maleadt · 2023-08-26T06:44:53Z

Lots of PkgEval breakage (500 packages crash now) that's probably related to this PR:

Internal error: encountered unexpected error in runtime:
MethodError(f=Base.string, args=(Expr(:call, :!==, :varstate, :nothing),), world=0x00000000000015b0)
jl_method_error_bare at /source/src/gf.c:2203
jl_method_error at /source/src/gf.c:2221
jl_lookup_generic_ at /source/src/gf.c:3052 [inlined]
ijl_apply_generic at /source/src/gf.c:3067
macro expansion at ./error.jl:231 [inlined]
construct_ssa! at ./compiler/ssair/slot2ssa.jl:752
slot2reg at ./compiler/optimize.jl:697
run_passes at ./compiler/optimize.jl:494
run_passes at ./compiler/optimize.jl:509 [inlined]
optimize at ./compiler/optimize.jl:458
jfptr_optimize_37748.1 at /opt/julia/lib/julia/sys.so (unknown line)
_jl_invoke at /source/src/gf.c:2870 [inlined]
ijl_apply_generic at /source/src/gf.c:3071
_typeinf at ./compiler/typeinfer.jl:265
typeinf at ./compiler/typeinfer.jl:212

This resolves a regression introduced in JuliaLang#50943 (comment) The problem was that we now expect the explicit CFG of the IR to correspond 1-1 in terms of reachability to the information we get from inference. That means that we have to unconditionally re-write control flow to match the branches that inference ended up actually exploring. The way I'm doing this here (by simply removing the check) I believe is valid because Inference has no way to represent "true or non-Bool" etc. If it did, we'd instead have to re-write these branches as a typeassert + a goto.

This resolves a regression introduced in JuliaLang#50943 (comment) The problem was that we now expect the explicit CFG of the IR to correspond 1-1 in terms of reachability to the information we get from inference. That means that we have to unconditionally re-write control flow to match the branches that inference ended up actually exploring. This change also modifies Inference to update the ssaflags on GotoIfNot and GotoNode statements, so that we can be sure these `@assert`s will trip if Inference is ever made smart enough to fold Union{Const(true), Float64}-style conditions.

This resolves a regression introduced in #50943 (comment) That PR requires the Goto/GotoIfNot statements of the IR to correspond 1-1 (in terms of reachability) to the information we get from inference. To make that happen, we have to unconditionally re-write control flow to match the branches that inference ended up actually exploring. The problem is that we were choosing not to do this if the GotoIfNot condition seemed to be maybe-non-Boolean. Thankfully, it turns out that check is unnecessary because Inference when unwrapping conditionals does not consider "true or non-Bool" etc. If it did, we'd instead have to re-write these branches as a `typeassert; goto` to encode the reachability

vtjnash · 2023-08-31T16:17:38Z

Could you look at why this seems to have broken performance of IdDict according to: https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_hash/5e5416a_vs_4ac6b05/report.md

(in the future, it would be good to runbenchmarks with the specifier ALL before merging significant changes, though that is as much a reminder to myself as a suggestion to you)

topolarity · 2023-09-06T16:21:30Z

Could you look at why this seems to have broken performance of IdDict according to: https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_hash/5e5416a_vs_4ac6b05/report.md

At least some of this appears to be due to increased precision on PhiNodes.

Previously in setindex!(::IdDict{Int, Int}, ::Any, ::Any) we were emitting:

%15 = φ (#4 => %14, #3 => val)::Any
...
%48 = π (%15, Int64)

Now we emit the improved:

%15 = φ (#4 => %14, #3 => val)::Int64

The problem is that codegen unboxes the refined PhiNode too eagerly and then has to re-box it for a function call
(kudos to @gbaraldi for identifying)

The unreachable here seems to be caused by the fact that (as of #50943) we re-use a more narrow type from Inference that correctly ignores these edges, but when inserting the `φᶜ` node in `slot2reg` we were including extra edges that never get exercised at runtime. I'm not sure _why_ this causes us to hit an unreachable, since the narrow type from inference is technically still valid (the catch block will never observe these spurious assignments at runtime), but this seems to fix the issue and anyway improves the quality of the IRCode generated by `slot2ssa`. Resolves #51159

TypedSlot no longer exists upstream, so we need to use a dummy type instead on the latest (master) builds of Julia.

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

topolarity requested a review from Keno August 16, 2023 18:40

topolarity changed the title ~~Rm typed slot~~ slot2ssa: remove TypedSlot Aug 16, 2023

topolarity added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Aug 16, 2023

topolarity commented Aug 17, 2023

View reviewed changes

topolarity force-pushed the rm-TypedSlot branch from b83d84f to 73cd069 Compare August 18, 2023 04:27

topolarity commented Aug 18, 2023

View reviewed changes

topolarity marked this pull request as ready for review August 18, 2023 04:33

topolarity requested a review from aviatesk August 18, 2023 04:35

Keno reviewed Aug 18, 2023

View reviewed changes

test/compiler/inference.jl Outdated Show resolved Hide resolved

Keno approved these changes Aug 18, 2023

View reviewed changes

topolarity added 6 commits August 18, 2023 05:25

Inference: Always update IR to avoid unreachable jumps

4bd8d35

This allows downstream consumers, like the optimizer, to be sure that they are only visiting blocks that Inference also visited. Also update tests and remove some unnused variables in `slot2ssa`.

test: Remove check for GotoIfNot

3b6d588

Now that type inference modifies branches to reflect static control flow information, we can't search for the branch that we want to optimize away.

Pangoraw reviewed Aug 18, 2023

View reviewed changes

base/compiler/ssair/slot2ssa.jl Outdated Show resolved Hide resolved

base/compiler/ssair/slot2ssa.jl Outdated Show resolved Hide resolved

topolarity linked an issue Aug 18, 2023 that may be closed by this pull request

Remove TypedSlot annotation in compiler #50299

Closed

topolarity force-pushed the rm-TypedSlot branch from 181c374 to 3b6d588 Compare August 18, 2023 21:50

topolarity commented Aug 18, 2023

View reviewed changes

base/compiler/typeinfer.jl Show resolved Hide resolved

aviatesk reviewed Aug 20, 2023

View reviewed changes

base/compiler/ssair/slot2ssa.jl Outdated Show resolved Hide resolved

aviatesk reviewed Aug 20, 2023

View reviewed changes

topolarity added 2 commits August 23, 2023 14:04

inference: Move unreachable branch updates to convert_to_ircode

d1f3980

This means that inference modifies types and not IR statements themselves. In order to make this happen, we pass have to pass per-statement reachability information to the optimizer. Also fixes up assorted review feedback.

aviatesk reviewed Aug 24, 2023

View reviewed changes

base/compiler/optimize.jl Outdated Show resolved Hide resolved

topolarity requested a review from aviatesk August 24, 2023 16:13

aviatesk added 2 commits August 25, 2023 13:41

fix up tests

afa7b37

perf improvements

e234e8f

aviatesk merged commit 5e5416a into JuliaLang:master Aug 25, 2023
4 of 7 checks passed

aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Aug 25, 2023

adjust to JuliaLang/julia#50943

ce16da3

aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Aug 25, 2023

adjust to JuliaLang/julia#50943

15bbbb8

aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Aug 25, 2023

TypedSyntax: adjust to JuliaLang/julia#50943

a488f07

aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Aug 25, 2023

TypedSyntax: adjust to JuliaLang/julia#50943 (#490)

e185fd7

aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Aug 25, 2023

adjust to JuliaLang/julia#50943

8885c4e

aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Aug 25, 2023

adjust to JuliaLang/julia#50943 (#585)

c8d1ef7

topolarity mentioned this pull request Aug 26, 2023

Always rewrite GotoIfNot with unreachable branches #51062

Merged

This was referenced Sep 1, 2023

Internal compiler error: UndefRefError during compaction #51144

Closed

Unreachable reached due to TypedSlot removal #51159

Closed

topolarity mentioned this pull request Sep 5, 2023

slot2ssa: Fix spurious φᶜ edges #51199

Merged

topolarity mentioned this pull request Sep 20, 2023

Codegen unboxes precise values too eagerly #51410

Open

Pangoraw mentioned this pull request Oct 15, 2023

Handle unreachable blocks in the adjoint CFG FluxML/Zygote.jl#1465

Merged

oxinabox mentioned this pull request Oct 17, 2023

Update to changes to construct_ssa JuliaDiff/Diffractor.jl#234

Merged

topolarity added a commit to topolarity/Libtask.jl that referenced this pull request Nov 8, 2023

Adjust to JuliaLang/julia#50943

d4b1e5e

TypedSlot no longer exists upstream, so we need to use a dummy type instead on the latest (master) builds of Julia.

yebai pushed a commit to TuringLang/Libtask.jl that referenced this pull request Nov 8, 2023

Adjust to JuliaLang/julia#50943 (#169)

a535130

TypedSlot no longer exists upstream, so we need to use a dummy type instead on the latest (master) builds of Julia.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

slot2ssa: remove `TypedSlot` #50943

slot2ssa: remove `TypedSlot` #50943

topolarity commented Aug 16, 2023 •

edited by aviatesk

Loading

topolarity Aug 17, 2023

Keno Aug 17, 2023

topolarity Aug 18, 2023

Keno Aug 18, 2023

topolarity commented Aug 18, 2023

Pangoraw left a comment •

edited

Loading

Keno commented Aug 18, 2023

aviatesk left a comment

aviatesk left a comment

aviatesk commented Aug 25, 2023

nanosoldier commented Aug 25, 2023

aviatesk commented Aug 25, 2023

aviatesk commented Aug 25, 2023

maleadt commented Aug 26, 2023

vtjnash commented Aug 31, 2023

topolarity commented Sep 6, 2023 •

edited

Loading

		@@ -1,13 +1,5 @@
		# This file is a part of Julia. License is MIT: https://julialang.org/license

		struct TypedSlot

slot2ssa: remove TypedSlot #50943

slot2ssa: remove TypedSlot #50943

Conversation

topolarity commented Aug 16, 2023 • edited by aviatesk Loading

topolarity Aug 17, 2023

Choose a reason for hiding this comment

Keno Aug 17, 2023

Choose a reason for hiding this comment

topolarity Aug 18, 2023

Choose a reason for hiding this comment

Keno Aug 18, 2023

Choose a reason for hiding this comment

topolarity commented Aug 18, 2023

Pangoraw left a comment • edited Loading

Choose a reason for hiding this comment

Keno commented Aug 18, 2023

aviatesk left a comment

Choose a reason for hiding this comment

aviatesk left a comment

Choose a reason for hiding this comment

aviatesk commented Aug 25, 2023

nanosoldier commented Aug 25, 2023

aviatesk commented Aug 25, 2023

aviatesk commented Aug 25, 2023

maleadt commented Aug 26, 2023

vtjnash commented Aug 31, 2023

topolarity commented Sep 6, 2023 • edited Loading

slot2ssa: remove `TypedSlot` #50943

slot2ssa: remove `TypedSlot` #50943

topolarity commented Aug 16, 2023 •

edited by aviatesk

Loading

Pangoraw left a comment •

edited

Loading

topolarity commented Sep 6, 2023 •

edited

Loading