Skip to content

Commit ec7a893

Browse files
aasulaasim
aasul
authored and
aasim
committed
opcache: preserve cv values across jit assignments and cse
1 parent 7091e5b commit ec7a893

7 files changed

Lines changed: 347 additions & 7 deletions

File tree

‎ext/opcache/jit/ir/ir_ra.c‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1910,6 +1910,73 @@ static int ir_try_swap_operands(ir_ctx *ctx, ir_ref i, ir_insn *insn)
19101910
return 0;
19111911
}
19121912

1913+
/* Reuse a bound COPY's storage when it covers the source's optimized lifetime. */
1914+
static void ir_share_copy_spill_slots(ir_ctx *ctx)
1915+
{
1916+
ir_ref ref;
1917+
1918+
if (!ctx->binding) {
1919+
return;
1920+
}
1921+
for (ref = 1; ref < ctx->insns_count; ref += ir_insn_len(&ctx->ir_base[ref])) {
1922+
ir_insn *insn = &ctx->ir_base[ref];
1923+
ir_ref source, binding;
1924+
ir_live_interval *source_interval, *copy_interval;
1925+
ir_use_list *uses;
1926+
ir_ref i;
1927+
ir_live_range range, *source_range;
1928+
1929+
if (insn->op != IR_COPY || insn->op2 != IR_COPY_HARD) {
1930+
continue;
1931+
}
1932+
source = insn->op1;
1933+
if (source <= 0 || source != ref - 1 || ctx->ir_base[source].op >= IR_PHI
1934+
|| ctx->ir_base[source].type != insn->type
1935+
|| ctx->cfg_map[source] != ctx->cfg_map[ref]
1936+
|| !ctx->vregs[source] || !ctx->vregs[ref]
1937+
|| ir_binding_find(ctx, source)) {
1938+
continue;
1939+
}
1940+
binding = ir_binding_find(ctx, ref);
1941+
if (!binding) {
1942+
continue;
1943+
}
1944+
source_interval = ctx->live_intervals[ctx->vregs[source]];
1945+
copy_interval = ctx->live_intervals[ctx->vregs[ref]];
1946+
/* Do not borrow storage from a coalesced value or across a loop entry. */
1947+
if (((source_interval->flags | copy_interval->flags) & IR_LIVE_INTERVAL_COALESCED)
1948+
|| source_interval->range.start < IR_START_LIVE_POS_FROM_REF(source)
1949+
|| source_interval->range.start >= IR_END_LIVE_POS_FROM_REF(source)
1950+
|| copy_interval->range.start < IR_START_LIVE_POS_FROM_REF(ref)
1951+
|| copy_interval->range.start >= IR_END_LIVE_POS_FROM_REF(ref)) {
1952+
continue;
1953+
}
1954+
/* Ignore only the adjacent source definition before the COPY. Every
1955+
* remaining source range, including holes and backedges, must be covered. */
1956+
range = source_interval->range;
1957+
range.start = copy_interval->range.start;
1958+
source_range = range.end > range.start ? &range : range.next;
1959+
if (source_range && !ir_ivals_inside(&copy_interval->range, source_range)) {
1960+
continue;
1961+
}
1962+
/* A source snapshot may require a different location from the bound COPY. */
1963+
uses = &ctx->use_lists[source];
1964+
for (i = 0; i < uses->count; i++) {
1965+
if (ctx->ir_base[ctx->use_edges[uses->refs + i]].op == IR_SNAPSHOT) {
1966+
break;
1967+
}
1968+
}
1969+
if (i != uses->count) {
1970+
continue;
1971+
}
1972+
/* Adjacent definitions have no intervening memory access or exit. The COPY's
1973+
* live ranges cover every later use of the identically typed source.
1974+
* Keep both identities and the allocator's binding compatibility checks. */
1975+
IR_ASSERT(binding < 0);
1976+
ir_bind(ctx, binding, source);
1977+
}
1978+
}
1979+
19131980
int ir_coalesce(ir_ctx *ctx)
19141981
{
19151982
uint32_t b, n, succ, pred_b, count = 0;
@@ -2110,6 +2177,8 @@ int ir_coalesce(ir_ctx *ctx)
21102177
ir_mem_free(xlat);
21112178
}
21122179

2180+
ir_share_copy_spill_slots(ctx);
2181+
21132182
return 1;
21142183
}
21152184

‎ext/opcache/jit/zend_jit_ir.c‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,7 +1387,13 @@ static void zend_jit_def_reg(zend_jit_ctx *jit, zend_jit_addr addr, ir_ref val)
13871387
val = ir_emit2(&jit->ctx, IR_OPT(IR_COPY, jit->ctx.ir_base[val].type), val, 1);
13881388
}
13891389
}
1390-
if (!zend_jit_spilling_may_cause_conflict(jit, var, val)) {
1390+
bool may_cause_conflict = zend_jit_spilling_may_cause_conflict(jit, var, val);
1391+
/* Keep CV identities separate even when the assignment cannot be bound. */
1392+
if (jit->ssa->vars[var].var < jit->current_op_array->last_var
1393+
&& jit->ctx.ir_base[val].op < IR_PHI) {
1394+
val = ir_emit2(&jit->ctx, IR_OPT(IR_COPY, jit->ctx.ir_base[val].type), val, IR_COPY_HARD);
1395+
}
1396+
if (!may_cause_conflict) {
13911397
val = ir_bind(&jit->ctx, -EX_NUM_TO_VAR(jit->ssa->vars[var].var), val);
13921398
}
13931399
}
@@ -6846,14 +6852,24 @@ static int zend_jit_assign_to_variable(zend_jit_ctx *jit,
68466852
ZEND_ASSERT(end_inputs->count == res_inputs->count || (end_inputs->count == 0 && res_inputs->count == 1));
68476853
jit->delay_var = -1;
68486854
jit->delay_refs = NULL;
6849-
if (res_inputs->count == 1) {
6850-
phi = res_inputs->refs[0];
6851-
} else {
6852-
phi = ir_PHI_N((var_def_info & MAY_BE_LONG & MAY_BE_LONG) ? IR_LONG : IR_DOUBLE,
6853-
res_inputs->count, res_inputs->refs);
6855+
/* Keep identical constants unbound: a COPY may be hoisted and spill into a live CV. */
6856+
phi = res_inputs->refs[0];
6857+
if (res_inputs->count > 1) {
6858+
uint32_t i = 1;
6859+
6860+
if (IR_IS_CONST_REF(phi)) {
6861+
while (i < res_inputs->count && res_inputs->refs[i] == phi) {
6862+
i++;
6863+
}
6864+
}
6865+
if (i < res_inputs->count) {
6866+
phi = ir_PHI_N((var_def_info & MAY_BE_LONG) ? IR_LONG : IR_DOUBLE,
6867+
res_inputs->count, res_inputs->refs);
6868+
}
68546869
}
68556870
if (Z_MODE(var_addr) == IS_REG) {
6856-
if ((var_info & (MAY_BE_REF|MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT|MAY_BE_RESOURCE)) || ref_addr) {
6871+
if (!IR_IS_CONST_REF(phi)
6872+
&& ((var_info & (MAY_BE_REF|MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT|MAY_BE_RESOURCE)) || ref_addr)) {
68576873
phi = ir_emit2(&jit->ctx, IR_OPT(IR_COPY, jit->ctx.ir_base[phi].type), phi, 1);
68586874
}
68596875
zend_jit_def_reg(jit, var_addr, phi);

‎ext/opcache/tests/jit/gh24683.phpt‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
--TEST--
2+
GH-24683 (Tracing JIT spills a hoisted constant into a live CV)
3+
--EXTENSIONS--
4+
opcache
5+
--SKIPIF--
6+
<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
7+
--INI--
8+
opcache.enable=1
9+
opcache.enable_cli=1
10+
opcache.jit=tracing
11+
opcache.jit_buffer_size=64M
12+
opcache.jit_hot_loop=61
13+
opcache.jit_hot_func=127
14+
opcache.jit_hot_return=8
15+
opcache.jit_hot_side_exit=8
16+
opcache.file_update_protection=0
17+
--FILE--
18+
<?php
19+
// php -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.jit_buffer_size=64M repro.php
20+
//
21+
// step() is a small state machine. Case 1 sets $ptr to a constant, uses it to
22+
// load the real pointer, and stores that in $ptr; it then falls through to case 2,
23+
// which reads through $ptr. After the tracing JIT has compiled the loop in
24+
// step(), $ptr sometimes still holds the constant when step() returns.
25+
26+
final class Bus
27+
{
28+
public int $value = 0;
29+
public function read(int $address): int { return $this->value; }
30+
}
31+
32+
function step(Bus $bus, array &$state, array &$ram, int $pc): int
33+
{
34+
$ptr = $state['ptr']; $i = $state['i']; $n = $state['n']; $ret = $state['ret'];
35+
$c = 0;
36+
while (true) {
37+
switch ($pc) {
38+
case 1:
39+
$ptr = 0x80080000;
40+
$a = $ptr - 0x6260;
41+
if (($a & 3) !== 0) { goto trap; }
42+
$p = $a & 0x1FFFFFFF;
43+
$inRam = $p < 0x00800000;
44+
$x = $inRam ? $ram[$p >> 2] : $bus->read($a);
45+
$ptr = $x; // 0x1F8010F4
46+
$t = $i !== $n; // unused, but needed to reproduce
47+
$c += 6; // unused, but needed to reproduce
48+
// fall through
49+
case 2:
50+
$a = $ptr;
51+
$p = $a & 0x1FFFFFFF;
52+
$inRam = $p < 0x00800000;
53+
$y = $inRam ? $ram[$p >> 2] : $bus->read($a);
54+
$pc = ($y & 0x7F000000) !== 0 ? 3 : 4;
55+
continue 2;
56+
case 3:
57+
if ($pc === 99) { goto trap; }
58+
$ret = 1;
59+
$pc = $ram[0]; // 5
60+
// fall through
61+
default:
62+
goto leave;
63+
}
64+
}
65+
leave:
66+
$state['ptr'] = $ptr; $state['ret'] = $ret;
67+
return $pc;
68+
trap:
69+
$state['ptr'] = $ptr;
70+
}
71+
72+
$bus = new Bus();
73+
$ram = array_fill(0, 0x80000, 0);
74+
$ram[0] = 5;
75+
$ram[0x79DA0 >> 2] = 0x1F8010F4;
76+
77+
$wrong = 0;
78+
for ($k = 0; $k < 10; $k++) {
79+
$bus->value = ($k % 128) << 24;
80+
$state = ['ptr' => 0x1F8010F4, 'i' => 0, 'n' => 0, 'ret' => 0];
81+
$pc = 2;
82+
for ($m = 0; $m < 100; $m++) {
83+
$pc = step($bus, $state, $ram, $pc);
84+
if ($state['ptr'] !== 0x1F8010F4) {
85+
printf("k=%d m=%d: \$ptr is 0x%X, expected 0x1F8010F4\n", $k, $m, $state['ptr']);
86+
$wrong++;
87+
}
88+
if ($pc !== 5) {
89+
break;
90+
}
91+
$pc = $state['ret'];
92+
}
93+
}
94+
echo "wrong: $wrong\n";
95+
--EXPECT--
96+
wrong: 0

‎ext/opcache/tests/jit/gh24683_cse.inc‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?php
2+
return function (array &$r, array &$ram, int $budget): int {
3+
$r4 = $r[4]; $r5 = $r[5]; $r14 = $r[14]; $r15 = $r[15]; $r24 = $r[24]; $r25 = $r[25]; $r29 = $r[29]; $r31 = $r[31];
4+
$flag = $r[0];
5+
$p = 0x108;
6+
$r14 = $ram[$p >> 2];
7+
$r29 = ($r29 - 24) & 0xFFFFFFFF;
8+
$a = $r29 + 20;
9+
$ram[$a >> 2] = $r31; // a different slot
10+
$a = $r14;
11+
if (($a & 0x7F800003) === 0) {
12+
}
13+
$r4 = $r14;
14+
if ($budget === 0) {
15+
return 1;
16+
}
17+
if (($a & 0x7F800003) === 0) {
18+
} else {
19+
if (!$flag) {
20+
}
21+
}
22+
$a = $r29 + 20;
23+
if (($a & 0x7F800003) === 0) {
24+
$r31 = $ram[$p >> 2]; // should read slot 0x108 >> 2 again
25+
}
26+
$r[4] = $r4; $r[5] = $r5; $r[14] = $r14; $r[15] = $r15; $r[24] = $r24; $r[25] = $r25; $r[29] = $r29; $r[31] = $r31;
27+
return $r31;
28+
};

‎ext/opcache/tests/jit/gh24683_cse.phpt‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
--TEST--
2+
GH-24683 (CSE result outlives its CV spill slot)
3+
--EXTENSIONS--
4+
opcache
5+
--SKIPIF--
6+
<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
7+
--INI--
8+
opcache.enable=1
9+
opcache.enable_cli=1
10+
opcache.jit=tracing
11+
opcache.jit_buffer_size=64M
12+
opcache.jit_hot_loop=61
13+
opcache.jit_hot_func=127
14+
opcache.jit_hot_return=8
15+
opcache.jit_hot_side_exit=8
16+
opcache.file_update_protection=0
17+
--FILE--
18+
<?php
19+
$fn = require __DIR__ . '/gh24683_cse.inc';
20+
$wrong = 0;
21+
for ($i = 0; $i < 1000; $i++) {
22+
$ram = array_fill(0, 0x80000, 0);
23+
$ram[0x108 >> 2] = 0xA000E1EC;
24+
$regs = array_fill(0, 32, 0);
25+
$regs[29] = 0x85D8;
26+
$regs[31] = 0xE10;
27+
if ($fn($regs, $ram, 58) !== 0xA000E1EC) { $wrong++; }
28+
}
29+
echo "$wrong of 1000 calls wrong\n";
30+
--EXPECT--
31+
0 of 1000 calls wrong

‎ext/opcache/tests/jit/gh24683_cse_float.phpt‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
--TEST--
2+
GH-24683 (Floating-point CSE survives a CV reassignment)
3+
--EXTENSIONS--
4+
opcache
5+
--SKIPIF--
6+
<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
7+
--INI--
8+
opcache.enable=1
9+
opcache.enable_cli=1
10+
opcache.jit=tracing
11+
opcache.jit_buffer_size=64M
12+
opcache.jit_hot_loop=61
13+
opcache.jit_hot_func=127
14+
opcache.jit_hot_return=8
15+
opcache.jit_hot_side_exit=8
16+
opcache.file_update_protection=0
17+
--FILE--
18+
<?php
19+
function pressure(array $input, array &$out, int $tag) {
20+
$x = $input[0];
21+
$a = $x + 11.25;
22+
$out[0] = $a;
23+
if ($tag === 91) {
24+
return -1;
25+
}
26+
$a = $input[1];
27+
$out[1] = $a;
28+
$a = $x + 11.25;
29+
$out[2] = $a;
30+
return $a;
31+
}
32+
33+
$wrong = 0;
34+
for ($k = 0; $k < 800; $k++) {
35+
$input = [$k % 13, 2];
36+
$out = [];
37+
$tag = ($k > 700 && $k % 17 === 0) ? 91 : 0;
38+
$result = pressure($input, $out, $tag);
39+
$value = $input[0] + 11.25;
40+
$expected = $tag === 91 ? [-1, [$value]] : [$value, [$value, 2, $value]];
41+
if ([$result, $out] !== $expected) {
42+
$wrong++;
43+
}
44+
}
45+
echo "wrong: $wrong\n";
46+
--EXPECT--
47+
wrong: 0

‎ext/opcache/tests/jit/gh24683_cse_pressure.phpt‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
--TEST--
2+
GH-24683 (Bound copies do not exhaust the tracing JIT spill frame)
3+
--EXTENSIONS--
4+
opcache
5+
--SKIPIF--
6+
<?php if (PHP_INT_SIZE != 8) die('skip requires 64-bit integers'); ?>
7+
--INI--
8+
opcache.enable=1
9+
opcache.enable_cli=1
10+
opcache.jit=tracing
11+
opcache.jit_buffer_size=64M
12+
; Isolate function-entry compilation from a caller loop that could fall back.
13+
opcache.jit_hot_loop=0
14+
opcache.jit_hot_func=127
15+
opcache.jit_hot_return=8
16+
opcache.jit_hot_side_exit=8
17+
opcache.file_update_protection=0
18+
opcache.jit_debug=0x14000
19+
--FILE--
20+
<?php
21+
function safe_pressure(array $in, array &$out, int $tag) {
22+
$x0 = $in[0];
23+
$x1 = $in[1];
24+
$x2 = $in[2];
25+
$x3 = $in[3];
26+
$x4 = $in[4];
27+
$a0 = $x0 + 11.25; $out[0] = $a0;
28+
$a1 = $x1 + 12.25; $out[1] = $a1;
29+
$a2 = $x2 + 13.25; $out[2] = $a2;
30+
$a3 = $x3 + 14.25; $out[3] = $a3;
31+
$a4 = $x4 + 15.25; $out[4] = $a4;
32+
if ($tag === 91) {
33+
return -1;
34+
}
35+
$b0 = $x0 + 11.25; $out[10] = $b0;
36+
$b1 = $x1 + 12.25; $out[11] = $b1;
37+
$b2 = $x2 + 13.25; $out[12] = $b2;
38+
$b3 = $x3 + 14.25; $out[13] = $b3;
39+
$b4 = $x4 + 15.25; $out[14] = $b4;
40+
return $a0 + $a1 + $a2 + $a3 + $a4;
41+
}
42+
43+
$hash = hash_init('sha256');
44+
for ($k = 0; $k < 800; $k++) {
45+
$in = range(1, 10);
46+
$in[0] = $k % 13;
47+
$out = [];
48+
$result = safe_pressure($in, $out, ($k > 700 && $k % 17 === 0) ? 91 : 0);
49+
hash_update($hash, serialize([$result, $out]));
50+
}
51+
echo hash_final($hash), "\n";
52+
--EXPECTREGEX--
53+
(?:---- TRACE [0-9]+ compiled\n\n)+01fee1fb9cedccbd27d2b9c70613b77b7095e42bdbcc80eb820337d461d21ec8

0 commit comments

Comments
 (0)