-
Notifications
You must be signed in to change notification settings - Fork 7.9k
re-use sprintf()
optimisation for printf()
#19658
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?
Conversation
There is one failing test now:
That test got introduced in https://bugs.php.net/bug.php?id=66251 and honestly I am not sure about what that test is actually testing ... OPcodes for the test before this PR:
and after:
|
I just see that the |
@dstogov do you have an idea why the
to
|
This is happening in pass1 php-src/Zend/Optimizer/pass1.c Lines 149 to 161 in d25687b
The problem seems to be that the ROPE optimisation removes the diff --git a/Zend/Optimizer/pass1.c b/Zend/Optimizer/pass1.c
index fe92db583fc..e83a87de5b8 100644
--- a/Zend/Optimizer/pass1.c
+++ b/Zend/Optimizer/pass1.c
@@ -264,6 +264,12 @@ void zend_optimizer_pass1(zend_op_array *op_array, zend_optimizer_ctx *ctx)
collect_constants = 0;
break;
}
+ case ZEND_DO_UCALL:
+ case ZEND_DO_FCALL:
+ case ZEND_DO_FCALL_BY_NAME:
+ collect_constants = 0;
+ break;
case ZEND_STRLEN:
if (opline->op1_type == IS_CONST &&
zend_optimizer_eval_strlen(&result, &ZEND_OP1_LITERAL(opline)) == SUCCESS) { solves the issue, although this might have side effects beyond my understanding .. I've applied this patch in 1526406 and CI is green, yet I fear that this has some consequences CI does not cover 😉 |
So as expected, with 1526406 the behaviour changes for <?php
echo getA();
const A="hello";
function getA() {return A;} from:
to:
|
/* Check for printf optimization from `zend_compile_func_printf()` | ||
* where the result of `printf()` is actually unused and remove the | ||
* superflous COPY_TMP, STRLEN and FREE opcodes: | ||
* T1 = COPY_TMP T0 | ||
* ECHO T0 | ||
* T2 = STRLEN T1 | ||
* FREE T2 | ||
*/ |
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.
I believe this is already handled by zend_optimize_cfg()
:
printf("%s\n", $a);
optimization_level=0:
0000 T2 = ROPE_INIT 3 string("A=")
0001 T2 = ROPE_ADD 1 T2 CV0($s)
0002 T1 = ROPE_END 2 T2 string("\n")
0003 T4 = COPY_TMP T1
0004 ECHO T1
0005 T5 = STRLEN T4
0006 FREE T5
0007 RETURN int(1)
optimization_level=$((1<<4)):
0000 T2 = ROPE_INIT 3 string("A=")
0001 T2 = ROPE_ADD 1 T2 CV0($s)
0002 T1 = ROPE_END 2 T2 string("\n")
0003 ECHO T1
0004 RETURN int(1)
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.
Did you remove my changes when testing this? Cause I can't get this result when I remove my code, also I do believe that these changes in the block_pass.c
are part of the optimisation level that you choose (the zend_optimize_cfg()
run)
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.
My bad, I did comment out your change before testing this but I think I didn't rebuild after that. So yeah what I observed was the result of your change :)
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.
An alternative would be to support ZEND_COPY_TMP
in dce.c
:
diff --git a/Zend/Optimizer/dce.c b/Zend/Optimizer/dce.c
index a00fd8bc6ad..bdc165de655 100644
--- a/Zend/Optimizer/dce.c
+++ b/Zend/Optimizer/dce.c
@@ -124,6 +124,7 @@ static inline bool may_have_side_effects(
case ZEND_FUNC_NUM_ARGS:
case ZEND_FUNC_GET_ARGS:
case ZEND_ARRAY_KEY_EXISTS:
+ case ZEND_COPY_TMP:
/* No side effects */
return 0;
case ZEND_FREE:
@@ -428,7 +429,9 @@ static bool dce_instr(context *ctx, zend_op *opline, zend_ssa_op *ssa_op) {
if ((opline->op1_type & (IS_VAR|IS_TMP_VAR))&& !is_var_dead(ctx, ssa_op->op1_use)) {
if (!try_remove_var_def(ctx, ssa_op->op1_use, ssa_op->op1_use_chain, opline)) {
if (may_be_refcounted(ssa->var_info[ssa_op->op1_use].type)
- && opline->opcode != ZEND_CASE && opline->opcode != ZEND_CASE_STRICT) {
+ && opline->opcode != ZEND_CASE
+ && opline->opcode != ZEND_CASE_STRICT
+ && opline->opcode != ZEND_COPY_TMP) {
free_var = ssa_op->op1_use;
free_var_type = opline->op1_type;
}
This PR piggybacks on #14546
test.php
:See
hyperfine
run, wherephp-orig
is without this patch:The generated opcodes before optimizer:
... and after optimizer: