-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Implement implicit move optimisation #11166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,7 +98,7 @@ typedef struct _sccp_ctx { | |
#define MAKE_TOP(zv) (Z_TYPE_INFO_P(zv) = TOP) | ||
#define MAKE_BOT(zv) (Z_TYPE_INFO_P(zv) = BOT) | ||
|
||
static void scp_dump_value(zval *zv) { | ||
static void scp_dump_value(const zval *zv) { | ||
if (IS_TOP(zv)) { | ||
fprintf(stderr, " top"); | ||
} else if (IS_BOT(zv)) { | ||
|
@@ -1050,6 +1050,12 @@ static void sccp_visit_instr(scdf_ctx *scdf, zend_op *opline, zend_ssa_op *ssa_o | |
case ZEND_SEND_VAL: | ||
case ZEND_SEND_VAR: | ||
{ | ||
SKIP_IF_TOP(op1); | ||
|
||
if (opline->opcode == ZEND_SEND_VAR) { | ||
SET_RESULT(op1, op1); | ||
} | ||
|
||
Comment on lines
+1053
to
+1058
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also add support for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably. That's somewhat separate from this PR, so do you want me to do it here or in another PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. SCCP didn't support |
||
/* If the value of a SEND for an ICALL changes, we need to reconsider the | ||
* ICALL result value. Otherwise we can ignore the opcode. */ | ||
zend_call_info *call; | ||
|
@@ -1058,7 +1064,7 @@ static void sccp_visit_instr(scdf_ctx *scdf, zend_op *opline, zend_ssa_op *ssa_o | |
} | ||
|
||
call = ctx->call_map[opline - ctx->scdf.op_array->opcodes]; | ||
if (IS_TOP(op1) || !call || !call->caller_call_opline | ||
if (!call || !call->caller_call_opline | ||
|| call->caller_call_opline->opcode != ZEND_DO_ICALL) { | ||
return; | ||
} | ||
|
@@ -2034,8 +2040,15 @@ static int remove_call(sccp_ctx *ctx, zend_op *opline, zend_ssa_op *ssa_op) | |
&ssa->ops[call->caller_init_opline - op_array->opcodes]); | ||
|
||
for (i = 0; i < call->num_args; i++) { | ||
zend_ssa_remove_instr(ssa, call->arg_info[i].opline, | ||
&ssa->ops[call->arg_info[i].opline - op_array->opcodes]); | ||
zend_op *op = call->arg_info[i].opline; | ||
zend_ssa_op *this_ssa_op = &ssa->ops[op - op_array->opcodes]; | ||
|
||
/* Not necessary to check ZEND_SEND_VAR_EX, because compile time calls don't support ZEND_SEND_VAR_EX. */ | ||
if (op->opcode == ZEND_SEND_VAR && this_ssa_op->op1_def >= 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose ZEND_SEND_VAR_EX is missed, because it can't be a part of a call to a "known" target. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, compile-time-calls cannot be done with ZEND_SEND_VAR_EX. I can add a comment to explain why this is though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No problem. I just liked to be sure. |
||
zend_ssa_replace_op1_def_by_op1_use(ssa, this_ssa_op); | ||
} | ||
|
||
zend_ssa_remove_instr(ssa, op, this_ssa_op); | ||
} | ||
|
||
// TODO: remove call_info completely??? | ||
|
@@ -2188,6 +2201,10 @@ static int try_remove_definition(sccp_ctx *ctx, int var_num, zend_ssa_var *var, | |
return 0; | ||
} | ||
|
||
if (opline->opcode == ZEND_SEND_VAR) { | ||
return 0; | ||
} | ||
|
||
/* Compound assign or incdec -> convert to direct ASSIGN */ | ||
|
||
if (!value) { | ||
|
@@ -2330,6 +2347,14 @@ static int replace_constant_operands(sccp_ctx *ctx) { | |
FOREACH_USE(var, use) { | ||
zend_op *opline = &op_array->opcodes[use]; | ||
zend_ssa_op *ssa_op = &ssa->ops[use]; | ||
/* Removing the def in try_remove_definition() may reduce optimisation opportunities. | ||
* We want to keep the no_val definition until we actually replace it with a constant. | ||
* For SEND_VAR_EX we can't replace the def because the instruction may pass an argument by ref, | ||
* which can make the variable a ref if the def is replaced by the use. */ | ||
if (opline->opcode == ZEND_SEND_VAR && ssa_op->op1_use == i && ssa_op->op1_def >= 0) { | ||
zend_ssa_replace_op1_def_by_op1_use(ssa, ssa_op); | ||
opline->extended_value = 0; | ||
} | ||
if (try_replace_op1(ctx, opline, ssa_op, i, value)) { | ||
if (opline->opcode == ZEND_NOP) { | ||
removed_ops++; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
<?php | ||
|
||
function array_merge_loop($n) { | ||
$a = range(1, 3); | ||
for ($i = 0; $i < $n; ++$i) { | ||
$a = array_merge($a, [1, 2, 3]); | ||
} | ||
} | ||
|
||
function array_merge_loop_with_temporary($n) { | ||
$a = []; | ||
for ($i = 0; $i < $n; ++$i) { | ||
$a = array_merge($a, [1, 2]) + [100]; | ||
} | ||
} | ||
|
||
function array_merge_loop_nested($n) { | ||
$a = []; | ||
for ($i = 0; $i < $n; ++$i) { | ||
$a = array_merge($a, [1, 2]) + [$a]; | ||
} | ||
} | ||
|
||
function array_unique_loop($n) { | ||
$a = range(1, 2000); | ||
$a += $a; | ||
for ($i = 0; $i < $n; ++$i) { | ||
$a = array_unique($a); | ||
} | ||
} | ||
|
||
function user_func_helper($array) { | ||
for ($i = 0; $i < 100; $i++) { | ||
$array[$i] = $i; | ||
} | ||
return $array; | ||
} | ||
|
||
function user_func($n) { | ||
$a = []; | ||
for ($i = 0; $i < $n; ++$i) { | ||
$a = user_func_helper($a); | ||
} | ||
} | ||
|
||
/*****/ | ||
|
||
require 'bench_common.php'; | ||
|
||
const N = 30000; | ||
|
||
$t0 = $t = start_test(); | ||
empty_loop(N); | ||
$t = end_test($t, 'empty_loop'); | ||
$overhead = $last_time; | ||
array_merge_loop(N); | ||
$t = end_test($t, 'array_merge', $overhead); | ||
array_merge_loop_with_temporary(N); | ||
$t = end_test($t, 'array_merge temp', $overhead); | ||
array_merge_loop_nested(N); | ||
$t = end_test($t, 'array_merge nest', $overhead); | ||
array_unique_loop(N); | ||
$t = end_test($t, 'array_unique', $overhead); | ||
user_func(N); | ||
$t = end_test($t, 'user_func', $overhead); | ||
total($t0, "Total"); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
<?php | ||
|
||
function empty_loop($n) { | ||
for ($i = 0; $i < $n; ++$i) { | ||
} | ||
} | ||
|
||
function gethrtime() | ||
{ | ||
$hrtime = hrtime(); | ||
return (($hrtime[0]*1000000000 + $hrtime[1]) / 1000000000); | ||
} | ||
|
||
function start_test() | ||
{ | ||
ob_start(); | ||
return gethrtime(); | ||
} | ||
|
||
function end_test($start, $name, $overhead = null) | ||
{ | ||
global $total; | ||
global $last_time; | ||
$end = gethrtime(); | ||
ob_end_clean(); | ||
$last_time = $end-$start; | ||
$total += $last_time; | ||
$num = number_format($last_time,3); | ||
$pad = str_repeat(" ", 24-strlen($name)-strlen($num)); | ||
if (is_null($overhead)) { | ||
echo $name.$pad.$num."\n"; | ||
} else { | ||
$num2 = number_format($last_time - $overhead,3); | ||
echo $name.$pad.$num." ".$num2."\n"; | ||
} | ||
ob_start(); | ||
return gethrtime(); | ||
} | ||
|
||
function total() | ||
{ | ||
global $total; | ||
$pad = str_repeat("-", 24); | ||
echo $pad."\n"; | ||
$num = number_format($total,3); | ||
$pad = str_repeat(" ", 24-strlen("Total")-strlen($num)); | ||
echo "Total".$pad.$num."\n"; | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much is this a problem - without it, will it corrupt something? Backtraces does normally create a CoW copy [1], so removing this condition can improve performance.
[1] https://3v4l.org/nbiOF/rfc#vgit.master_jit and https://3v4l.org/gSHDb/rfc#vgit.master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not cause memory corruption, but causes a semantic change.
Let's say you remove the condition, then the following can happen:
If you have
$a = array_merge($a, ...);
where $a is an argument, and the call causes an exception, then $a will be NULL in the backtrace because its value has been moved into the function call argument.