JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.73k stars 5.48k forks source link

optimizer: `lifting_cache` retrieval is broken #43927

Open aviatesk opened 2 years ago

aviatesk commented 2 years ago

https://github.com/JuliaLang/julia/blob/a400a24a5d9e6609740814e4092538feb499ce60/base/compiler/ssair/passes.jl#L579-L584

Since the key type of lifting_cache is Pair{AnySSAValue, Any}, this query of cache key existence is broken, and so lifting_cache is actually never be used at this moment. Though the fix doesn't seem to be that easy. I tried the following diff, but failed to bootstrap.

diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl
index 51b5ec076f..137985ac8e 100644
--- a/base/compiler/ssair/passes.jl
+++ b/base/compiler/ssair/passes.jl
@@ -552,15 +552,15 @@ function perform_lifting!(compact::IncrementalCompact,
     # Insert PhiNodes
     lifted_phis = LiftedPhi[]
     for item in visited_phinodes
-        # FIXME this cache retrieval is obviously broken
-        if (item, cache_key) in keys(lifting_cache)
-            ssa = lifting_cache[Pair{AnySSAValue, Any}(item, cache_key)]
+        ckey = Pair{AnySSAValue, Any}(item, cache_key)
+        if ckey in keys(lifting_cache)
+            ssa = lifting_cache[ckey]
             push!(lifted_phis, LiftedPhi(ssa, compact[ssa]::PhiNode, false))
             continue
         end
         n = PhiNode()
         ssa = insert_node!(compact, item, effect_free(NewInstruction(n, result_t)))
-        lifting_cache[Pair{AnySSAValue, Any}(item, cache_key)] = ssa
+        lifting_cache[ckey] = ssa
         push!(lifted_phis, LiftedPhi(ssa, n, true))
     end

/cc @Keno

vtjnash commented 2 years ago

Is this a regression, or just slow?

aviatesk commented 2 years ago

AFAICT, this has been broken since implemented. So it's definitely not a regression but a room for an improvement.