Merge pull request #29262 from JuliaLang/jn/interpret-phi

interpreter: fix bugs with ssair phi-node handling

Author: vtjnash

base/stacktraces.jl

@@ -123,7 +123,7 @@ const top_level_scope_sym = Symbol("top-level scope")
 using Base.Meta
 is_loc_meta(expr, kind) = isexpr(expr, :meta) && length(expr.args) >= 1 && expr.args[1] === kind
 function lookup(ip::Base.InterpreterIP)
-    if ip.code isa Core.MethodInstance
+    if ip.code isa Core.MethodInstance && ip.code.def isa Method
         codeinfo = ip.code.inferred
         func = ip.code.def.name
         file = ip.code.def.file

base/tuple.jl

@@ -21,8 +21,8 @@ firstindex(@nospecialize t::Tuple) = 1
 lastindex(@nospecialize t::Tuple) = length(t)
 size(@nospecialize(t::Tuple), d) = (d == 1) ? length(t) : throw(ArgumentError("invalid tuple dimension $d"))
 axes(@nospecialize t::Tuple) = OneTo(length(t))
-@eval getindex(t::Tuple, i::Int) = getfield(t, i, $(Expr(:boundscheck)))
-@eval getindex(t::Tuple, i::Real) = getfield(t, convert(Int, i), $(Expr(:boundscheck)))
+@eval getindex(@nospecialize(t::Tuple), i::Int) = getfield(t, i, $(Expr(:boundscheck)))
+@eval getindex(@nospecialize(t::Tuple), i::Real) = getfield(t, convert(Int, i), $(Expr(:boundscheck)))
 getindex(t::Tuple, r::AbstractArray{<:Any,1}) = ([t[ri] for ri in r]...,)
 getindex(t::Tuple, b::AbstractArray{Bool,1}) = length(b) == length(t) ? getindex(t, findall(b)) : throw(BoundsError(t, b))
 getindex(t::Tuple, c::Colon) = t
@@ -38,7 +38,10 @@ _setindex(v, i::Integer) = ()
 
 ## iterating ##
 
-iterate(t::Tuple, i::Int=1) = 1 <= i <= length(t) ? (@inbounds t[i], i+1) : nothing
+function iterate(@nospecialize(t::Tuple), i::Int=1)
+    @_inline_meta
+    return (1 <= i <= length(t)) ? (@inbounds t[i], i + 1) : nothing
+end
 
 keys(@nospecialize t::Tuple) = OneTo(length(t))
 

src/interpreter.c

@@ -20,16 +20,15 @@ typedef struct {
     jl_module_t *module; // context for globals
     jl_value_t **locals; // slots for holding local slots and ssavalues
     jl_svec_t *sparam_vals; // method static parameters, if eval-ing a method body
-    size_t last_branch; // Points at the last branch statement (for evaluating phi nodes)
-    size_t ip; // Points to the currently-evaluating statement
-    int preevaluation; // use special rules for pre-evaluating expressions
+    size_t ip; // Leak the currently-evaluating statement index to backtrace capture
+    int preevaluation; // use special rules for pre-evaluating expressions (deprecated--only for ccall handling)
     int continue_at; // statement index to jump to after leaving exception handler (0 if none)
 } interpreter_state;
 
 #include "interpreter-stacktrace.c"
 
 static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s);
-static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s, int start, int toplevel);
+static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s, size_t ip, int toplevel);
 
 int jl_is_toplevel_only_expr(jl_value_t *e);
 
@@ -362,8 +361,6 @@ SECT_INTERP static void eval_stmt_value(jl_value_t *stmt, interpreter_state *s)
 {
     jl_value_t *res = eval_value(stmt, s);
     s->locals[jl_source_nslots(s->src) + s->ip] = res;
-    if (!jl_is_phinode(stmt))
-        s->last_branch = s->ip;
 }
 
 SECT_INTERP static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s)
@@ -403,23 +400,7 @@ SECT_INTERP static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s)
 #endif
         return val;
     }
-    if (jl_is_phinode(e)) {
-        jl_array_t *edges = (jl_array_t*)jl_fieldref_noalloc(e, 0);
-        ssize_t edge = -1;
-        for (int i = 0; i < jl_array_len(edges); ++i) {
-            size_t from = jl_unbox_long(jl_arrayref(edges, i));
-            if (from == s->last_branch + 1) {
-                edge = i;
-                break;
-            }
-        }
-        if (edge == -1) {
-            // edges list doesn't contain last branch. this value should be unused.
-            return NULL;
-        }
-        jl_value_t *val = jl_array_ptr_ref((jl_array_t*)jl_fieldref_noalloc(e, 1), edge);
-        return eval_value(val, s);
-    }
+    assert(!jl_is_phinode(e) && !jl_is_phicnode(e) && !jl_is_upsilonnode(e) && "malformed AST");
     if (!jl_is_expr(e))
         return e;
     jl_expr_t *ex = (jl_expr_t*)e;
@@ -523,26 +504,115 @@ SECT_INTERP static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s)
     abort();
 }
 
-SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s, int start, int toplevel)
+// phi nodes don't behave like proper instructions, so we require a special interpreter to handle them
+SECT_INTERP static size_t eval_phi(jl_array_t *stmts, interpreter_state *s, size_t ns, size_t to)
+{
+    size_t from = s->ip;
+    size_t ip = to;
+    unsigned nphi = 0;
+    for (ip = to; ip < ns; ip++) {
+        jl_value_t *e = jl_array_ptr_ref(stmts, ip);
+        if (!jl_is_phinode(e))
+            break;
+        nphi += 1;
+    }
+    if (nphi) {
+        jl_value_t **dest = &s->locals[jl_source_nslots(s->src) + to];
+        jl_value_t **phis; // = (jl_value_t**)alloca(sizeof(jl_value_t*) * nphi);
+        JL_GC_PUSHARGS(phis, nphi);
+        for (unsigned i = 0; i < nphi; i++) {
+            jl_value_t *e = jl_array_ptr_ref(stmts, to + i);
+            assert(jl_is_phinode(e));
+            jl_array_t *edges = (jl_array_t*)jl_fieldref_noalloc(e, 0);
+            ssize_t edge = -1;
+            size_t closest = to; // implicit edge has `to <= edge - 1 < to + i`
+            // this is because we could see the following IR (all 1-indexed):
+            //   goto %3 unless %cond
+            //   %2 = phi ...
+            //   %3 = phi (1)[1 => %a], (2)[2 => %b]
+            // from = 1, to = closest = 2, i = 1 --> edge = 2, edge_from = 2, from = 2
+            for (unsigned j = 0; j < jl_array_len(edges); ++j) {
+                size_t edge_from = jl_unbox_long(jl_arrayref(edges, j)); // 1-indexed
+                if (edge_from == from + 1) {
+                    if (edge == -1)
+                        edge = j;
+                }
+                else if (closest < edge_from && edge_from < (to + i + 1)) {
+                    // if we found a nearer implicit branch from fall-through,
+                    // that occurred since the last explicit branch,
+                    // we should use the value from that edge instead
+                    edge = j;
+                    closest = edge_from;
+                }
+            }
+            jl_value_t *val = NULL;
+            unsigned n_oldphi = closest - to;
+            if (n_oldphi) {
+                // promote this implicit branch to a basic block start
+                // and move all phi values to their position in edges
+                // note that we might have already processed some phi nodes
+                // in this basic block, so we need to be extra careful here
+                // to ignore those
+                for (unsigned j = 0; j < n_oldphi; j++) {
+                    dest[j] = phis[j];
+                }
+                for (unsigned j = n_oldphi; j < i; j++) {
+                    // move the rest to the start of phis
+                    phis[j - n_oldphi] = phis[j];
+                    phis[j] = NULL;
+                }
+                from = closest - 1;
+                i -= n_oldphi;
+                dest += n_oldphi;
+                to += n_oldphi;
+                nphi -= n_oldphi;
+            }
+            if (edge != -1) {
+                // if edges list doesn't contain last branch, this value should be unused.
+                jl_array_t *values = (jl_array_t*)jl_fieldref_noalloc(e, 1);
+                val = jl_array_ptr_ref(values, edge);
+                val = eval_value(val, s);
+            }
+            phis[i] = val;
+        }
+        // now move all phi values to their position in edges
+        for (unsigned j = 0; j < nphi; j++) {
+            dest[j] = phis[j];
+        }
+        JL_GC_POP();
+    }
+    return ip;
+}
+
+SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s, size_t ip, int toplevel)
 {
     jl_handler_t __eh;
-    s->ip = start;
     size_t ns = jl_array_len(stmts);
 
     while (1) {
-        if (s->ip >= ns)
+        s->ip = ip;
+        if (ip >= ns)
             jl_error("`body` expression must terminate in `return`. Use `block` instead.");
         if (toplevel)
             jl_get_ptls_states()->world_age = jl_world_counter;
-        jl_value_t *stmt = jl_array_ptr_ref(stmts, s->ip);
+        jl_value_t *stmt = jl_array_ptr_ref(stmts, ip);
+        assert(!jl_is_phinode(stmt));
+        size_t next_ip = ip + 1;
+        assert(!jl_is_phinode(stmt) && !jl_is_phicnode(stmt) && "malformed AST");
         if (jl_is_gotonode(stmt)) {
-            s->last_branch = s->ip;
-            s->ip = jl_gotonode_label(stmt) - 1;
-            continue;
+            next_ip = jl_gotonode_label(stmt) - 1;
+        }
+        else if (jl_is_upsilonnode(stmt)) {
+            jl_value_t *val = jl_fieldref_noalloc(stmt, 0);
+            if (val)
+                val = eval_value(val, s);
+            jl_value_t *phic = s->locals[jl_source_nslots(s->src) + ip];
+            assert(jl_is_ssavalue(phic));
+            ssize_t id = ((jl_ssavalue_t*)phic)->id - 1;
+            s->locals[jl_source_nslots(s->src) + id] = val;
         }
         else if (jl_is_expr(stmt)) {
             // Most exprs are allowed to end a BB by fall through
-            s->last_branch = s->ip;
             jl_sym_t *head = ((jl_expr_t*)stmt)->head;
             assert(head != unreachable_sym);
             if (head == return_sym) {
@@ -554,7 +624,7 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s
                 if (jl_is_slot(lhs)) {
                     ssize_t n = jl_slot_number(lhs);
                     assert(n <= jl_source_nslots(s->src) && n > 0);
-                    s->locals[n-1] = rhs;
+                    s->locals[n - 1] = rhs;
                 }
                 else {
                     jl_module_t *modu;
@@ -577,8 +647,7 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s
             else if (head == goto_ifnot_sym) {
                 jl_value_t *cond = eval_value(jl_exprarg(stmt, 0), s);
                 if (cond == jl_false) {
-                    s->ip = jl_unbox_long(jl_exprarg(stmt, 1)) - 1;
-                    continue;
+                    next_ip = jl_unbox_long(jl_exprarg(stmt, 1)) - 1;
                 }
                 else if (cond != jl_true) {
                     jl_type_error_rt("toplevel", "if", (jl_value_t*)jl_bool_type, cond);
@@ -605,29 +674,32 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s
                     for (size_t i = 0; i < jl_array_len(values); ++i) {
                         jl_value_t *val = jl_array_ptr_ref(values, i);
                         assert(jl_is_ssavalue(val));
-                        s->locals[jl_source_nslots(s->src) + ((jl_ssavalue_t*)val)->id - 1] = jl_box_ssavalue(catch_ip);
+                        size_t upsilon = ((jl_ssavalue_t*)val)->id - 1;
+                        assert(jl_is_upsilonnode(jl_array_ptr_ref(stmts, upsilon)));
+                        s->locals[jl_source_nslots(s->src) + upsilon] = jl_box_ssavalue(catch_ip + 1);
                     }
+                    s->locals[jl_source_nslots(s->src) + catch_ip] = NULL;
                     catch_ip += 1;
                 }
-                if (!jl_setjmp(__eh.eh_ctx,1)) {
-                    return eval_body(stmts, s, s->ip + 1, toplevel);
+                if (!jl_setjmp(__eh.eh_ctx, 1)) {
+                    return eval_body(stmts, s, next_ip, toplevel);
                 }
-                else if (s->continue_at) {
-                    s->ip = s->continue_at;
+                else if (s->continue_at) { // means we reached a :leave expression
+                    ip = s->continue_at;
                     s->continue_at = 0;
                     continue;
                 }
-                else {
+                else { // a real exeception
 #ifdef _OS_WINDOWS_
                     if (jl_get_ptls_states()->exception_in_transit == jl_stackovf_exception)
                         _resetstkoflw();
 #endif
-                    s->ip = jl_unbox_long(jl_exprarg(stmt, 0)) - 1;
+                    ip = catch_ip;
                     continue;
                 }
             }
             else if (head == leave_sym) {
-                int hand_n_leave = jl_unbox_long(jl_exprarg(stmt,0));
+                int hand_n_leave = jl_unbox_long(jl_exprarg(stmt, 0));
                 assert(hand_n_leave > 0);
                 // equivalent to jl_pop_handler(hand_n_leave) :
                 jl_ptls_t ptls = jl_get_ptls_states();
@@ -636,7 +708,7 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s
                     eh = eh->prev;
                 jl_eh_restore_state(eh);
                 // pop jmp_bufs from stack
-                s->continue_at = s->ip + 1;
+                s->continue_at = next_ip;
                 jl_longjmp(eh->eh_ctx, 1);
             }
             else if (head == const_sym) {
@@ -683,7 +755,6 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s
             }
         }
         else if (jl_is_newvarnode(stmt)) {
-            s->last_branch = s->ip;
             jl_value_t *var = jl_fieldref(stmt, 0);
             assert(jl_is_slot(var));
             ssize_t n = jl_slot_number(var);
@@ -696,7 +767,7 @@ SECT_INTERP static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s
         else {
             eval_stmt_value(stmt, s);
         }
-        s->ip++;
+        ip = eval_phi(stmts, s, ns, next_ip);
     }
     abort();
 }
@@ -766,6 +837,7 @@ SECT_INTERP CALLBACK_ABI void *jl_interpret_call_callback(interpreter_state *s,
     }
     s->locals = locals + 2;
     s->sparam_vals = args->lam->sparam_vals;
+    s->preevaluation = 0;
     s->continue_at = 0;
     s->mi = args->lam;
     size_t i;

test/choosetests.jl

@@ -107,7 +107,7 @@ function choosetests(choices = [])
         prepend!(tests, ["subarray"])
     end
 
-    compilertests = ["compiler/compiler", "compiler/validation"]
+    compilertests = ["compiler/compiler", "compiler/validation", "compiler/ssair"]
 
     if "compiler" in skip_tests
         filter!(x -> (x != "compiler" && !(x in compilertests)), tests)

test/compiler/compiler.jl

@@ -954,13 +954,14 @@ let f, m
     f() = 0
     m = first(methods(f))
     m.source = Base.uncompressed_ast(m)::CodeInfo
-    m.source.ssavaluetypes = 3
-    m.source.codelocs = Int32[1, 1, 1]
     m.source.code = Any[
         Expr(:call, GlobalRef(Core, :svec), 1, 2, 3),
         Expr(:call, Core._apply, GlobalRef(Base, :+), SSAValue(1)),
         Expr(:return, SSAValue(2))
     ]
+    nstmts = length(m.source.code)
+    m.source.ssavaluetypes = nstmts
+    m.source.codelocs = fill(Int32(1), nstmts)
     @test @inferred(f()) == 6
 end