Skip to content

Conversation

@ianatol
Copy link
Member

@ianatol ianatol commented Mar 29, 2022

In #44557, we fixed some places where SSAValue uses were miscounted. During the development of that PR, after an egregious amount of printlns, we devised a framework to test our calculated use counts against an "oracle" use count independently derived from the relevant IncrementalCompact.

This PR cleans that framework up a bit and adds an option to toggle it (its off by default of course). Hopefully this framework means that no one has to manually check these counts again if/when we decide to make related changes.

Turning this on during our compiler tests shows we do still overcount at times. Overcounting is a lot less bad than undercounting (which was the crux of an issue with #44557 where we were deleting actually used instructions), but it may mean that we are missing some opportunities to do dead code elimination.

For example, the following comes almost immediately after starting Base.runtests("compiler"):

julia> compact.result.inst
29-element Vector{Any}:
 :(Base.getfield(_3, 1, $(Expr(:boundscheck))))
 :(Base.getfield(_2, :itr))
 :(Base.bitcast(UInt64, 1))
 :(Base.sub_int(%3, 0x0000000000000001))
 :(Base.arraylen(%2))
 :(Base.sle_int(0, %5))
 :(Base.bitcast(UInt64, %5))
 :(Base.ult_int(%4, %7))
 :(Base.and_int(%6, %8))
 :(goto %3 if not %9)
 :(Base.arrayref(false, %2, 1))
 :(Base.add_int(1, 1))
 :(Core.tuple(%11, %12))
 :(goto %4)
 :(Base.nothing)
 :(goto %4)
 :(φ (%2 => false, %3 => true))
 :(φ (%2 => %11))
 :(φ (%2 => %12))
 :(φ (%3 => %15))
 :(goto %5)
 :(goto %7 if not %17)
 Core.PiNode(:(%20), Core.Const(nothing))
 :(return %23)
 :(Core.tuple(%1, %18))
 :(Base.add_int(%1, 1))
 :(Core.tuple(%26, %19))
 :(Core.tuple(%25, %27))
 :(return %28)

julia> compact.used_ssas[13]
1

julia> oracle_used_ssas[13]
0

In this case, our oracle shows that %13 = :(Core.tuple(%11, %12)) is unused (which we can visually confirm), but it has not been eliminated by DCE because we think it has a use somewhere.

@ianatol ianatol added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Mar 29, 2022
@ianatol ianatol requested a review from Keno March 29, 2022 21:44
@Keno
Copy link
Member

Keno commented Mar 30, 2022

Concept LGTM. One DRY comment on the implementation.

@ianatol ianatol force-pushed the ia/ssacounter branch 4 times, most recently from ec95383 to 7312685 Compare March 31, 2022 16:04
@ianatol ianatol force-pushed the ia/ssacounter branch 4 times, most recently from f9686d0 to 995f2f4 Compare April 1, 2022 17:24
@ianatol
Copy link
Member Author

ianatol commented Apr 4, 2022

Gonna add some basic userefs tests just to make sure there's nothing else along those lines here

@ianatol ianatol force-pushed the ia/ssacounter branch 2 times, most recently from 6cce43c to 574291f Compare April 5, 2022 02:32
@ianatol
Copy link
Member Author

ianatol commented May 14, 2022

@Keno I think this is ready now --- the interface to actually use the checking framework is a bit clunky, but I'm not sure of a better way to throw such an error from inside Core.Compiler.

Eventually, once we've gotten SSA counting to a place where the oracle check consistently passes, I'd like to add some tests that utilize `oracle_check

@ianatol ianatol added the merge me PR is reviewed. Merge when all tests are passing label May 16, 2022
@KristofferC KristofferC merged commit bad3e39 into JuliaLang:master May 17, 2022
@ianatol ianatol removed the merge me PR is reviewed. Merge when all tests are passing label May 17, 2022
@ianatol ianatol deleted the ia/ssacounter branch May 17, 2022 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants