Relax constraints on the PHI block

In #50158, I tought the verifier to reject code that has invalid
statements in the original PHI block. In #50235, this required
irinterp to stop folding PhiNodes to the respective constants.
I said at the time that a subsequent compact would fix it, but
it turns out that we don't actually have the logic for that.
I might still add that logic, but on the other hand it just
seems kinda silly that PhiNodes need to be a special case here.

This PR relaxes the semantics of the PHI block, to allow any
value-position constant to appear in the PHI block and undoes
the irinterp change from #50235. Only the interpreter really
cares about the semantics of the phi block, so the primary change
is there.

Of note, SSAValue forwards are not allowed in the phi block. This
is because of the following:

```
loop:
 %1 = %(...)
 %2 = %1
 %3 = %(top => %1)
```

The two phi values %1 and %2 have different semantics: %1 gets
the *current* iteration of the loop, while %3 gets the *previous*
value. As a result, any pass that wants to move SSAValues out of
PhiNode uses would have to be aware of these semantics anyway,
and there's no simplicitly benefits to allowing SSAValues in the
middle of a phi block.

Author: Keno

base/compiler/ssair/irinterp.jl

@@ -162,7 +162,7 @@ function reprocess_instruction!(interp::AbstractInterpreter, idx::Int, bb::Union
     if rt !== nothing
         if isa(rt, Const)
             ir.stmts[idx][:type] = rt
-            if is_inlineable_constant(rt.val) && !isa(inst, PhiNode) && (ir.stmts[idx][:flag] & IR_FLAG_EFFECT_FREE) != 0
+            if is_inlineable_constant(rt.val) && (ir.stmts[idx][:flag] & IR_FLAG_EFFECT_FREE) != 0
                 ir.stmts[idx][:inst] = quoted(rt.val)
             end
             return true

base/compiler/ssair/verify.jl

@@ -20,6 +20,7 @@ if !isdefined(@__MODULE__, Symbol("@verify_error"))
     end
 end
 
+is_value_pos_expr_head(head::Symbol) = head === :boundscheck
 function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int, use_idx::Int, printed_use_idx::Int, print::Bool, isforeigncall::Bool, arg_idx::Int, allow_frontend_forms::Bool)
     if isa(op, SSAValue)
         if op.id > length(ir.stmts)
@@ -60,7 +61,7 @@ function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int,
             # Allow a tuple in symbol position for foreigncall - this isn't actually
             # a real call - it's interpreted in global scope by codegen. However,
             # we do need to keep this a real use, because it could also be a pointer.
-        elseif op.head !== :boundscheck
+        elseif !is_value_pos_expr_head(op.head)
             if !allow_frontend_forms || op.head !== :opaque_closure_method
                 @verify_error "Expr not allowed in value position"
                 error("")
@@ -189,9 +190,11 @@ function verify_ir(ir::IRCode, print::Bool=true,
     end
     lastbb = 0
     is_phinode_block = false
+    firstidx = 1
     for (bb, idx) in bbidxiter(ir)
         if bb != lastbb
             is_phinode_block = true
+            firstidx = idx
             lastbb = bb
         end
         # We allow invalid IR in dead code to avoid passes having to detect when
@@ -244,12 +247,19 @@ function verify_ir(ir::IRCode, print::Bool=true,
                 check_op(ir, domtree, val, Int(edge), last(ir.cfg.blocks[stmt.edges[i]].stmts)+1, idx, print, false, i, allow_frontend_forms)
             end
             continue
-        elseif stmt === nothing
-            # Nothing to do
-            continue
         end
 
-        is_phinode_block = false
+        if is_phinode_block && isa(stmt, Union{Expr, UpsilonNode, PhiCNode, SSAValue})
+            if !isa(stmt, Expr) || !is_value_pos_expr_head(stmt.head)
+                # Go back and check that all non-PhiNodes are valid value-position
+                for validate_idx in firstidx:(idx-1)
+                    validate_stmt = ir.stmts[validate_idx][:inst]
+                    isa(validate_stmt, PhiNode) && continue
+                    check_op(ir, domtree, op, validate_stmt, idx, idx, print, isforeigncall, 0, allow_frontend_forms)
+                end
+                is_phinode_block = false
+            end
+        end
         if isa(stmt, PhiCNode)
             for i = 1:length(stmt.values)
                 val = stmt.values[i]

src/interpreter.c

@@ -349,20 +349,34 @@ static size_t eval_phi(jl_array_t *stmts, interpreter_state *s, size_t ns, size_
 {
     size_t from = s->ip;
     size_t ip = to;
-    unsigned nphi = 0;
+    unsigned nphiblockstmts = 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 (!jl_is_phinode(e)) {
+            if (jl_is_expr(e) || jl_is_returnnode(e) || jl_is_gotoifnot(e) ||
+                jl_is_gotonode(e) || jl_is_phicnode(e) || jl_is_upsilonnode(e) ||
+                jl_is_ssavalue(e)) {
+                break;
+            }
+            // Everything else is allowed in the phi-block for implementation
+            // convenience - fall through.
+        }
+        nphiblockstmts += 1;
     }
-    if (nphi) {
+    if (nphiblockstmts) {
         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 **phis; // = (jl_value_t**)alloca(sizeof(jl_value_t*) * nphiblockstmts);
+        JL_GC_PUSHARGS(phis, nphiblockstmts);
+        for (unsigned i = 0; i < nphiblockstmts; i++) {
             jl_value_t *e = jl_array_ptr_ref(stmts, to + i);
-            assert(jl_is_phinode(e));
+            if (!jl_is_phinode(e)) {
+                // IR verification guarantees that the only thing that gets
+                // evaluated here are constants, so it doesn't matter if we
+                // update the locals or the phis, but let's be consistent
+                // for simplicity.
+                phis[i] = eval_value(e, s);
+                continue;
+            }
             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`
@@ -405,7 +419,7 @@ static size_t eval_phi(jl_array_t *stmts, interpreter_state *s, size_t ns, size_
                 i -= n_oldphi;
                 dest += n_oldphi;
                 to += n_oldphi;
-                nphi -= n_oldphi;
+                nphiblockstmts -= n_oldphi;
             }
             if (edge != -1) {
                 // if edges list doesn't contain last branch, or the value is explicitly undefined
@@ -418,7 +432,7 @@ static size_t eval_phi(jl_array_t *stmts, interpreter_state *s, size_t ns, size_
             phis[i] = val;
         }
         // now move all phi values to their position in edges
-        for (unsigned j = 0; j < nphi; j++) {
+        for (unsigned j = 0; j < nphiblockstmts; j++) {
             dest[j] = phis[j];
         }
         JL_GC_POP();