-
Notifications
You must be signed in to change notification settings - Fork 8k
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 2 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,14 @@ 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]; | ||
|
|
||
| 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_op1_use(ssa, this_ssa_op); | ||
| } | ||
|
|
||
| zend_ssa_remove_instr(ssa, op, this_ssa_op); | ||
| } | ||
|
|
||
| // TODO: remove call_info completely??? | ||
|
|
@@ -2188,6 +2200,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 +2346,12 @@ 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. */ | ||
| if (opline->opcode == ZEND_SEND_VAR && ssa_op->op1_use == i && ssa_op->op1_def >= 0) { | ||
| zend_ssa_replace_op1_def_op1_use(ssa, ssa_op); | ||
| opline->extended_value = 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. This should be done 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. I tried this, I can't do it. This is because SEND_VAR_EX can pass a variable as a reference. Then if we replace the def with the use then the variable at the callsite will become a ref. See for example Zend/tests/method_argument_binding.phpt |
||
| 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 |
|---|---|---|
|
|
@@ -703,6 +703,10 @@ static zend_always_inline int _zend_ssa_rename_op(const zend_op_array *op_array, | |
| } | ||
| break; | ||
| case ZEND_SEND_VAR: | ||
| if (opline->op1_type == IS_CV && ((build_flags & ZEND_SSA_RC_INFERENCE) || opline->op2_type == IS_UNUSED)) { | ||
| goto add_op1_def; | ||
| } | ||
| break; | ||
| case ZEND_CAST: | ||
| case ZEND_QM_ASSIGN: | ||
| case ZEND_JMP_SET: | ||
|
|
@@ -1680,3 +1684,13 @@ void zend_ssa_rename_var_uses(zend_ssa *ssa, int old, int new, bool update_types | |
| old_var->phi_use_chain = NULL; | ||
| } | ||
| /* }}} */ | ||
|
|
||
| void zend_ssa_replace_op1_def_op1_use(zend_ssa *ssa, zend_ssa_op *ssa_op) | ||
|
||
| { | ||
| int op1_new = ssa_op->op1_use; | ||
| ZEND_ASSERT(op1_new >= 0); | ||
| ZEND_ASSERT(ssa_op->op1_def >= 0); | ||
| /* zend_ssa_rename_var_uses() clear use_chain & phi_use_chain for us */ | ||
| zend_ssa_rename_var_uses(ssa, ssa_op->op1_def, op1_new, true); | ||
| zend_ssa_remove_op1_def(ssa, ssa_op); | ||
| } | ||
| 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.