OPcache: preserve constant and reusable expression assignment values

ext/opcache/jit/ir/ir_ra.c

@@ -1910,6 +1910,73 @@ static int ir_try_swap_operands(ir_ctx *ctx, ir_ref i, ir_insn *insn)
 	return 0;
 }
 
+/* Reuse a bound COPY's storage when it covers the source's optimized lifetime. */
+static void ir_share_copy_spill_slots(ir_ctx *ctx)
+{
+	ir_ref ref;
+
+	if (!ctx->binding) {
+		return;
+	}
+	for (ref = 1; ref < ctx->insns_count; ref += ir_insn_len(&ctx->ir_base[ref])) {
+		ir_insn *insn = &ctx->ir_base[ref];
+		ir_ref source, binding;
+		ir_live_interval *source_interval, *copy_interval;
+		ir_use_list *uses;
+		ir_ref i;
+		ir_live_range range, *source_range;
+
+		if (insn->op != IR_COPY || insn->op2 != IR_COPY_HARD) {
+			continue;
+		}
+		source = insn->op1;
+		if (source <= 0 || source != ref - 1 || ctx->ir_base[source].op >= IR_PHI
+		 || ctx->ir_base[source].type != insn->type
+		 || ctx->cfg_map[source] != ctx->cfg_map[ref]
+		 || !ctx->vregs[source] || !ctx->vregs[ref]
+		 || ir_binding_find(ctx, source)) {
+			continue;
+		}
+		binding = ir_binding_find(ctx, ref);
+		if (!binding) {
+			continue;
+		}
+		source_interval = ctx->live_intervals[ctx->vregs[source]];
+		copy_interval = ctx->live_intervals[ctx->vregs[ref]];
+		/* Do not borrow storage from a coalesced value or across a loop entry. */
+		if (((source_interval->flags | copy_interval->flags) & IR_LIVE_INTERVAL_COALESCED)
+		 || source_interval->range.start < IR_START_LIVE_POS_FROM_REF(source)
+		 || source_interval->range.start >= IR_END_LIVE_POS_FROM_REF(source)
+		 || copy_interval->range.start < IR_START_LIVE_POS_FROM_REF(ref)
+		 || copy_interval->range.start >= IR_END_LIVE_POS_FROM_REF(ref)) {
+			continue;
+		}
+		/* Ignore only the adjacent source definition before the COPY. Every
+		 * remaining source range, including holes and backedges, must be covered. */
+		range = source_interval->range;
+		range.start = copy_interval->range.start;
+		source_range = range.end > range.start ? &range : range.next;
+		if (source_range && !ir_ivals_inside(&copy_interval->range, source_range)) {
+			continue;
+		}
+		/* A source snapshot may require a different location from the bound COPY. */
+		uses = &ctx->use_lists[source];
+		for (i = 0; i < uses->count; i++) {
+			if (ctx->ir_base[ctx->use_edges[uses->refs + i]].op == IR_SNAPSHOT) {
+				break;
+			}
+		}
+		if (i != uses->count) {
+			continue;
+		}
+		/* Adjacent definitions have no intervening memory access or exit. The COPY's
+		 * live ranges cover every later use of the identically typed source.
+		 * Keep both identities and the allocator's binding compatibility checks. */
+		IR_ASSERT(binding < 0);
+		ir_bind(ctx, binding, source);
+	}
+}
+
 int ir_coalesce(ir_ctx *ctx)
 {
 	uint32_t b, n, succ, pred_b, count = 0;
@@ -2110,6 +2177,8 @@ int ir_coalesce(ir_ctx *ctx)
 		ir_mem_free(xlat);
 	}
 
+	ir_share_copy_spill_slots(ctx);
+
 	return 1;
 }
 

ext/opcache/jit/zend_jit_ir.c

@@ -1378,7 +1378,13 @@ static void zend_jit_def_reg(zend_jit_ctx *jit, zend_jit_addr addr, ir_ref val)
 				val = ir_emit2(&jit->ctx, IR_OPT(IR_COPY, jit->ctx.ir_base[val].type), val, 1);
 			}
 		}
-		if (!zend_jit_spilling_may_cause_conflict(jit, var, val)) {
+		bool may_cause_conflict = zend_jit_spilling_may_cause_conflict(jit, var, val);
+		/* Keep CV identities separate even when the assignment cannot be bound. */
+		if (jit->ssa->vars[var].var < jit->current_op_array->last_var
+		 && jit->ctx.ir_base[val].op < IR_PHI) {
+			val = ir_emit2(&jit->ctx, IR_OPT(IR_COPY, jit->ctx.ir_base[val].type), val, IR_COPY_HARD);
+		}
+		if (!may_cause_conflict) {
 			val = ir_bind(&jit->ctx, -EX_NUM_TO_VAR(jit->ssa->vars[var].var), val);
 		}
 	}
@@ -6828,14 +6834,24 @@ static int zend_jit_assign_to_variable(zend_jit_ctx   *jit,
 		ZEND_ASSERT(end_inputs->count == res_inputs->count || (end_inputs->count == 0 && res_inputs->count == 1));
 		jit->delay_var = -1;
 		jit->delay_refs = NULL;
-		if (res_inputs->count == 1) {
-			phi = res_inputs->refs[0];
-		} else {
-			phi = ir_PHI_N((var_def_info & MAY_BE_LONG & MAY_BE_LONG) ? IR_LONG : IR_DOUBLE,
-				res_inputs->count, res_inputs->refs);
+		/* Keep identical constants unbound: a COPY may be hoisted and spill into a live CV. */
+		phi = res_inputs->refs[0];
+		if (res_inputs->count > 1) {
+			uint32_t i = 1;
+
+			if (IR_IS_CONST_REF(phi)) {
+				while (i < res_inputs->count && res_inputs->refs[i] == phi) {
+					i++;
+				}
+			}
+			if (i < res_inputs->count) {
+				phi = ir_PHI_N((var_def_info & MAY_BE_LONG) ? IR_LONG : IR_DOUBLE,
+					res_inputs->count, res_inputs->refs);
+			}
 		}
 		if (Z_MODE(var_addr) == IS_REG) {
-			if ((var_info & (MAY_BE_REF|MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT|MAY_BE_RESOURCE)) || ref_addr) {
+			if (!IR_IS_CONST_REF(phi)
+			 && ((var_info & (MAY_BE_REF|MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT|MAY_BE_RESOURCE)) || ref_addr)) {
 				phi = ir_emit2(&jit->ctx, IR_OPT(IR_COPY, jit->ctx.ir_base[phi].type), phi, 1);
 			}
 			zend_jit_def_reg(jit, var_addr, phi);

ext/opcache/tests/jit/gh24683.phpt

@@ -0,0 +1,96 @@
+--TEST--
+GH-24683 (Tracing JIT spills a hoisted constant into a live CV)
+--EXTENSIONS--
+opcache
+--SKIPIF--
+<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.jit=tracing
+opcache.jit_buffer_size=64M
+opcache.jit_hot_loop=61
+opcache.jit_hot_func=127
+opcache.jit_hot_return=8
+opcache.jit_hot_side_exit=8
+opcache.file_update_protection=0
+--FILE--
+<?php
+// php -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.jit_buffer_size=64M repro.php
+//
+// step() is a small state machine. Case 1 sets $ptr to a constant, uses it to
+// load the real pointer, and stores that in $ptr; it then falls through to case 2,
+// which reads through $ptr. After the tracing JIT has compiled the loop in
+// step(), $ptr sometimes still holds the constant when step() returns.
+
+final class Bus
+{
+    public int $value = 0;
+    public function read(int $address): int { return $this->value; }
+}
+
+function step(Bus $bus, array &$state, array &$ram, int $pc): int
+{
+    $ptr = $state['ptr']; $i = $state['i']; $n = $state['n']; $ret = $state['ret'];
+    $c = 0;
+    while (true) {
+        switch ($pc) {
+            case 1:
+                $ptr = 0x80080000;
+                $a = $ptr - 0x6260;
+                if (($a & 3) !== 0) { goto trap; }
+                $p = $a & 0x1FFFFFFF;
+                $inRam = $p < 0x00800000;
+                $x = $inRam ? $ram[$p >> 2] : $bus->read($a);
+                $ptr = $x;              // 0x1F8010F4
+                $t = $i !== $n;         // unused, but needed to reproduce
+                $c += 6;                // unused, but needed to reproduce
+                // fall through
+            case 2:
+                $a = $ptr;
+                $p = $a & 0x1FFFFFFF;
+                $inRam = $p < 0x00800000;
+                $y = $inRam ? $ram[$p >> 2] : $bus->read($a);
+                $pc = ($y & 0x7F000000) !== 0 ? 3 : 4;
+                continue 2;
+            case 3:
+                if ($pc === 99) { goto trap; }
+                $ret = 1;
+                $pc = $ram[0];          // 5
+                // fall through
+            default:
+                goto leave;
+        }
+    }
+    leave:
+    $state['ptr'] = $ptr; $state['ret'] = $ret;
+    return $pc;
+    trap:
+    $state['ptr'] = $ptr;
+}
+
+$bus = new Bus();
+$ram = array_fill(0, 0x80000, 0);
+$ram[0] = 5;
+$ram[0x79DA0 >> 2] = 0x1F8010F4;
+
+$wrong = 0;
+for ($k = 0; $k < 10; $k++) {
+    $bus->value = ($k % 128) << 24;
+    $state = ['ptr' => 0x1F8010F4, 'i' => 0, 'n' => 0, 'ret' => 0];
+    $pc = 2;
+    for ($m = 0; $m < 100; $m++) {
+        $pc = step($bus, $state, $ram, $pc);
+        if ($state['ptr'] !== 0x1F8010F4) {
+            printf("k=%d m=%d: \$ptr is 0x%X, expected 0x1F8010F4\n", $k, $m, $state['ptr']);
+            $wrong++;
+        }
+        if ($pc !== 5) {
+            break;
+        }
+        $pc = $state['ret'];
+    }
+}
+echo "wrong: $wrong\n";
+--EXPECT--
+wrong: 0

ext/opcache/tests/jit/gh24683_cse.inc

@@ -0,0 +1,28 @@
+<?php
+return function (array &$r, array &$ram, int $budget): int {
+    $r4 = $r[4]; $r5 = $r[5]; $r14 = $r[14]; $r15 = $r[15]; $r24 = $r[24]; $r25 = $r[25]; $r29 = $r[29]; $r31 = $r[31];
+    $flag = $r[0];
+    $p = 0x108;
+    $r14 = $ram[$p >> 2];
+    $r29 = ($r29 - 24) & 0xFFFFFFFF;
+    $a = $r29 + 20;
+    $ram[$a >> 2] = $r31;          // a different slot
+    $a = $r14;
+    if (($a & 0x7F800003) === 0) {
+    }
+    $r4 = $r14;
+    if ($budget === 0) {
+        return 1;
+    }
+    if (($a & 0x7F800003) === 0) {
+    } else {
+        if (!$flag) {
+        }
+    }
+    $a = $r29 + 20;
+    if (($a & 0x7F800003) === 0) {
+        $r31 = $ram[$p >> 2];          // should read slot 0x108 >> 2 again
+    }
+    $r[4] = $r4; $r[5] = $r5; $r[14] = $r14; $r[15] = $r15; $r[24] = $r24; $r[25] = $r25; $r[29] = $r29; $r[31] = $r31;
+    return $r31;
+};

ext/opcache/tests/jit/gh24683_cse.phpt

@@ -0,0 +1,31 @@
+--TEST--
+GH-24683 (CSE result outlives its CV spill slot)
+--EXTENSIONS--
+opcache
+--SKIPIF--
+<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.jit=tracing
+opcache.jit_buffer_size=64M
+opcache.jit_hot_loop=61
+opcache.jit_hot_func=127
+opcache.jit_hot_return=8
+opcache.jit_hot_side_exit=8
+opcache.file_update_protection=0
+--FILE--
+<?php
+$fn = require __DIR__ . '/gh24683_cse.inc';
+$wrong = 0;
+for ($i = 0; $i < 1000; $i++) {
+    $ram = array_fill(0, 0x80000, 0);
+    $ram[0x108 >> 2] = 0xA000E1EC;
+    $regs = array_fill(0, 32, 0);
+    $regs[29] = 0x85D8;
+    $regs[31] = 0xE10;
+    if ($fn($regs, $ram, 58) !== 0xA000E1EC) { $wrong++; }
+}
+echo "$wrong of 1000 calls wrong\n";
+--EXPECT--
+0 of 1000 calls wrong

ext/opcache/tests/jit/gh24683_cse_float.phpt

@@ -0,0 +1,47 @@
+--TEST--
+GH-24683 (Floating-point CSE survives a CV reassignment)
+--EXTENSIONS--
+opcache
+--SKIPIF--
+<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.jit=tracing
+opcache.jit_buffer_size=64M
+opcache.jit_hot_loop=61
+opcache.jit_hot_func=127
+opcache.jit_hot_return=8
+opcache.jit_hot_side_exit=8
+opcache.file_update_protection=0
+--FILE--
+<?php
+function pressure(array $input, array &$out, int $tag) {
+    $x = $input[0];
+    $a = $x + 11.25;
+    $out[0] = $a;
+    if ($tag === 91) {
+        return -1;
+    }
+    $a = $input[1];
+    $out[1] = $a;
+    $a = $x + 11.25;
+    $out[2] = $a;
+    return $a;
+}
+
+$wrong = 0;
+for ($k = 0; $k < 800; $k++) {
+    $input = [$k % 13, 2];
+    $out = [];
+    $tag = ($k > 700 && $k % 17 === 0) ? 91 : 0;
+    $result = pressure($input, $out, $tag);
+    $value = $input[0] + 11.25;
+    $expected = $tag === 91 ? [-1, [$value]] : [$value, [$value, 2, $value]];
+    if ([$result, $out] !== $expected) {
+        $wrong++;
+    }
+}
+echo "wrong: $wrong\n";
+--EXPECT--
+wrong: 0

ext/opcache/tests/jit/gh24683_cse_pressure.phpt

@@ -0,0 +1,53 @@
+--TEST--
+GH-24683 (Bound copies do not exhaust the tracing JIT spill frame)
+--EXTENSIONS--
+opcache
+--SKIPIF--
+<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.jit=tracing
+opcache.jit_buffer_size=64M
+; Isolate function-entry compilation from a caller loop that could fall back.
+opcache.jit_hot_loop=0
+opcache.jit_hot_func=127
+opcache.jit_hot_return=8
+opcache.jit_hot_side_exit=8
+opcache.file_update_protection=0
+opcache.jit_debug=0x14000
+--FILE--
+<?php
+function safe_pressure(array $in, array &$out, int $tag) {
+    $x0 = $in[0];
+    $x1 = $in[1];
+    $x2 = $in[2];
+    $x3 = $in[3];
+    $x4 = $in[4];
+    $a0 = $x0 + 11.25; $out[0] = $a0;
+    $a1 = $x1 + 12.25; $out[1] = $a1;
+    $a2 = $x2 + 13.25; $out[2] = $a2;
+    $a3 = $x3 + 14.25; $out[3] = $a3;
+    $a4 = $x4 + 15.25; $out[4] = $a4;
+    if ($tag === 91) {
+        return -1;
+    }
+    $b0 = $x0 + 11.25; $out[10] = $b0;
+    $b1 = $x1 + 12.25; $out[11] = $b1;
+    $b2 = $x2 + 13.25; $out[12] = $b2;
+    $b3 = $x3 + 14.25; $out[13] = $b3;
+    $b4 = $x4 + 15.25; $out[14] = $b4;
+    return $a0 + $a1 + $a2 + $a3 + $a4;
+}
+
+$hash = hash_init('sha256');
+for ($k = 0; $k < 800; $k++) {
+    $in = range(1, 10);
+    $in[0] = $k % 13;
+    $out = [];
+    $result = safe_pressure($in, $out, ($k > 700 && $k % 17 === 0) ? 91 : 0);
+    hash_update($hash, serialize([$result, $out]));
+}
+echo hash_final($hash), "\n";
+--EXPECTREGEX--
+(?:---- TRACE [0-9]+ compiled\n\n)+01fee1fb9cedccbd27d2b9c70613b77b7095e42bdbcc80eb820337d461d21ec8