Introduce improved SROA pass #27126

Keno · 2018-05-16T19:12:49Z

This is a rebased and fixed version of the improved SROA pass from #26778.
There's a decent piece of new infrastructure wrapped up in this: The ability
to insert new nodes during compaction. This is a bit tricky because it requires
tracking which version of the statements buffer a given SSAValue belongs to.
At the moment this is done mostly manually, but I'm hoping to clean that up
in the future. The idea of the new SROA pass is fairly straightforward:
Given a use of an interesting value, it traces through all phi nodes, finding
all leaves, applies whatever transformation to those leaves and then re-inserts
a phi nest corresponding to the phi nest of the original value.

This is a rebased and fixed version of the improved SROA pass from #26778. There's a decent piece of new infrastructure wrapped up in this: The ability to insert new nodes during compaction. This is a bit tricky because it requires tracking which version of the statements buffer a given SSAValue belongs to. At the moment this is done mostly manually, but I'm hoping to clean that up in the future. The idea of the new SROA pass is fairly straightforward: Given a use of an interesting value, it traces through all phi nodes, finding all leaves, applies whatever transformation to those leaves and then re-inserts a phi nest corresponding to the phi nest of the original value.

Keno · 2018-05-16T19:18:23Z

I apologize in advance for some of the messiness in this PR, but since it's on the critical path for 0.7alpha, my plan is to merge this as soon as acceptable by CI (and nanosoldier), so we can merge #25261 and tag the alpha. There's a whole bunch of cleanup to be done after.

Keno · 2018-05-16T19:18:56Z

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

vtjnash · 2018-05-16T23:05:48Z

base/compiler/ssair/verify.jl

+    elseif isa(op, Union{OldSSAValue, NewSSAValue})
+        #@Base.show ir
+        @verify_error "Left over SSA marker"
+        error()


need to fix these error() calls before release

This is in the verifier.

yes, that's why I said before release, not before merge

vtjnash · 2018-05-16T23:11:01Z

base/compiler/ssair/ir.jl

+end
+
+function resort_pending!(compact)
+    sort!(compact.pending_perm, DEFAULT_STABLE, Order.By(x->compact.pending_nodes[x].pos))


is this supposed to be returning compact.pending_perm?

vtjnash · 2018-05-16T23:11:34Z

base/compiler/ssair/ir.jl

+    sort!(compact.pending_perm, DEFAULT_STABLE, Order.By(x->compact.pending_nodes[x].pos))
+end
+
+function insert_node!(compact::IncrementalCompact, before, @nospecialize(typ), @nospecialize(val), attach_after::Bool=false)


type signature, or use dispatch

vtjnash · 2018-05-16T23:11:54Z

base/compiler/ssair/ir.jl

 end

-function insert_node_here!(compact::IncrementalCompact, @nospecialize(val), @nospecialize(typ), ltable_idx::Int)
+function insert_node_here!(compact::IncrementalCompact, @nospecialize(val), @nospecialize(typ), ltable_idx::Int, reverse_affinity=false)


type signature

vtjnash · 2018-05-16T23:13:05Z

base/compiler/ssair/ir.jl

+    # Add count for new use
+    if count_added_node!(compact, v)
+        push!(compact.late_fixup, idx.id)
+    end
 end


setindex! should return first argument

vtjnash · 2018-05-16T23:13:14Z

base/compiler/ssair/ir.jl

+    # Add count for new use
+    if count_added_node!(compact, v)
+        push!(compact.late_fixup, idx.id)
+    end
 end

 function setindex!(compact::IncrementalCompact, @nospecialize(v), idx::Int)


setindex! should return first argument

vtjnash · 2018-05-16T23:13:38Z

base/compiler/ssair/ir.jl

@@ -509,10 +621,14 @@ end

 function getindex(view::TypesView, idx)


type-signature

vtjnash · 2018-05-16T23:14:15Z

base/compiler/ssair/ir.jl

@@ -528,57 +644,114 @@ function done(compact::IncrementalCompact, (idx, _a)::Tuple{Int, Int})
    return idx > length(compact.ir.stmts) && (compact.new_nodes_idx > length(compact.perm))
 end

+function getindex(view::TypesView, idx::NewSSAValue)
+    @assert isa(view.ir, IncrementalCompact)
+    compact = view.ir


use type-assert here for better performance and better error messages

vtjnash · 2018-05-16T23:15:25Z

base/compiler/ssair/ir.jl

+        return process_newnode!(compact, new_idx, new_node_entry, idx, active_bb, true)
+    elseif !isempty(compact.pending_perm) &&
+        (entry = compact.pending_nodes[compact.pending_perm[1]];
+         entry.attach_after ? entry.pos == idx - 1 : entry.pos == idx)


can we make the control flow here less convoluted?

vtjnash · 2018-05-16T23:15:55Z

base/compiler/ssair/ir.jl

@@ -673,22 +853,29 @@ function next(compact::IncrementalCompact, (idx, active_bb)::Tuple{Int, Int})
    return Pair{Int, Any}(old_result_idx, compact.result[old_result_idx]), (compact.idx, active_bb)
 end

-function maybe_erase_unused!(extra_worklist, compact, idx)
-    effect_free = stmt_effect_free(compact.result[idx], compact, compact.ir.mod)
+function maybe_erase_unused!(extra_worklist, compact, idx, callback = x->nothing)


type-signature

nanosoldier · 2018-05-17T00:21:49Z

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

Keno · 2018-05-17T00:38:10Z

Looks fine. I'll merge this and address the review in a follow up PR.

vtjnash · 2018-05-28T17:31:55Z

What's the PR number of the followup? This PR re-introduced many of the performance issues common to the rest of this new code (failing to annotate types in argument signatures, using enumerate, etc.). It's hard to improve compiler performance it we can't even do the simple stuff. (eventually we also need to do the harder work too, like not using IdDict, but I understand that will require more rework).

Keno · 2018-05-28T18:27:15Z

Still on my todo list. There were a number of correctness issues to address first.

This has been there since #27126 (9100329) Dudging by the use of `reverse_mapping[stmt_val]` below this should either be a branch condition or an assert.

* Use the field index passed in in `lift_leaves` The caller has already done all the computation including bound checking. The `field` computed in this function is also affecting all the following iterations which is almost certainly wrong. * Remove unnecessary type check on `field` in `lift_leaves` since it is always `Int` * Move a branch disabling `return nothing` higher up * Remove some duplicated calculation on field index in `getfield_elim_pass!` * Fix `try_compute_fieldidx` to return `nothing` for non-`Int` `Integer` field index. This can cause `getfield_nothrow` to return incorrect result. It also gives the caller worse type info about the return value. * Teach `getfield_nothrow` that `isbits` field cannot be undefined and getfield on such field cannot throw. This is already handled in `isdefined_tfunc`. ---- Ref #26948 (fa02d34) Ref #27126 (9100329)

* Use the field index passed in in `lift_leaves` The caller has already done all the computation including bound checking. The `field` computed in this function is also affecting all the following iterations which is almost certainly wrong. * Remove unnecessary type check on `field` in `lift_leaves` since it is always `Int` * Move a branch disabling `return nothing` higher up * Remove some duplicated calculation on field index in `getfield_elim_pass!` * Fix `try_compute_fieldidx` to return `nothing` for non-`Int` `Integer` field index. This can cause `getfield_nothrow` to return incorrect result. It also gives the caller worse type info about the return value. * Teach `getfield_nothrow` that `isbits` field cannot be undefined and getfield on such field cannot throw. This is already handled in `isdefined_tfunc`. * Fix a few wrong use of `isbits` in dead branches ---- Ref #26948 (fa02d34) Ref #27126 (9100329)

This has been there since #27126 (9100329) Dudging by the use of `reverse_mapping[stmt_val]` below this should either be a branch condition or an assert.

* Use the field index passed in in `lift_leaves` The caller has already done all the computation including bound checking. The `field` computed in this function is also affecting all the following iterations which is almost certainly wrong. * Remove unnecessary type check on `field` in `lift_leaves` since it is always `Int` * Move a branch disabling `return nothing` higher up * Remove some duplicated calculation on field index in `getfield_elim_pass!` * Fix `try_compute_fieldidx` to return `nothing` for non-`Int` `Integer` field index. This can cause `getfield_nothrow` to return incorrect result. It also gives the caller worse type info about the return value. * Teach `getfield_nothrow` that `isbits` field cannot be undefined and getfield on such field cannot throw. This is already handled in `isdefined_tfunc`. * Fix a few wrong use of `isbits` in dead branches ---- Ref #26948 (fa02d34) Ref #27126 (9100329)

This has been there since JuliaLang#27126 (9100329) Dudging by the use of `reverse_mapping[stmt_val]` below this should either be a branch condition or an assert.

* Use the field index passed in in `lift_leaves` The caller has already done all the computation including bound checking. The `field` computed in this function is also affecting all the following iterations which is almost certainly wrong. * Remove unnecessary type check on `field` in `lift_leaves` since it is always `Int` * Move a branch disabling `return nothing` higher up * Remove some duplicated calculation on field index in `getfield_elim_pass!` * Fix `try_compute_fieldidx` to return `nothing` for non-`Int` `Integer` field index. This can cause `getfield_nothrow` to return incorrect result. It also gives the caller worse type info about the return value. * Teach `getfield_nothrow` that `isbits` field cannot be undefined and getfield on such field cannot throw. This is already handled in `isdefined_tfunc`. * Fix a few wrong use of `isbits` in dead branches ---- Ref JuliaLang#26948 (fa02d34) Ref JuliaLang#27126 (9100329)

Keno mentioned this pull request May 16, 2018

WIP: [NewOptimizer] The one SROA pass to rule them all #26778

Closed

vtjnash reviewed May 16, 2018

View reviewed changes

Keno merged commit d44944d into master May 17, 2018

ararslan deleted the kf/sroarebase branch May 17, 2018 02:57

yuyichao added a commit that referenced this pull request Sep 5, 2020

Fix Dict key check in type inference

cf74ff2

This has been there since #27126 (9100329) Dudging by the use of `reverse_mapping[stmt_val]` below this should either be a branch condition or an assert.

yuyichao added a commit that referenced this pull request Sep 5, 2020

Fix Dict key check in type inference

570dc1b

This has been there since #27126 (9100329) Dudging by the use of `reverse_mapping[stmt_val]` below this should either be a branch condition or an assert.

yuyichao mentioned this pull request Sep 5, 2020

Fix Dict key check in type inference #37418

Closed

yuyichao mentioned this pull request Sep 6, 2020

Some getfield related fixes in inference #37423

Closed

Keno pushed a commit that referenced this pull request Dec 30, 2020

Fix Dict key check in type inference

bb2e065

This has been there since #27126 (9100329) Dudging by the use of `reverse_mapping[stmt_val]` below this should either be a branch condition or an assert.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce improved SROA pass #27126

Introduce improved SROA pass #27126

Keno commented May 16, 2018

Keno commented May 16, 2018

Keno commented May 16, 2018

vtjnash May 16, 2018

Keno May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

vtjnash May 16, 2018

nanosoldier commented May 17, 2018

Keno commented May 17, 2018

vtjnash commented May 28, 2018

Keno commented May 28, 2018

		@@ -509,10 +621,14 @@ end

		function getindex(view::TypesView, idx)

Introduce improved SROA pass #27126

Introduce improved SROA pass #27126

Conversation

Keno commented May 16, 2018

Keno commented May 16, 2018

Keno commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented May 17, 2018

Keno commented May 17, 2018

vtjnash commented May 28, 2018

Keno commented May 28, 2018