Elide ZEND_VERIFY_RETURN_TYPE when returning $this if possible#21948
Elide ZEND_VERIFY_RETURN_TYPE when returning $this if possible#21948Girgias wants to merge 4 commits intophp:masterfrom
Conversation
Girgias
force-pushed
the
elide-return-type-check-for-this-with-static
branch
from
e874ad3 to
bf784bd
Compare
Girgias
changed the title
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
Looks correct otherwise. No strong opinions, this might probably also be improved in the optimizer instead.
| * then we don't need to check the return type */ | ||
| const zend_op_array *op_array = CG(active_op_array); | ||
| if (expr && op_array->last >= 1 | ||
| && op_array->opcodes[op_array->last-1].opcode == ZEND_FETCH_THIS |
There was a problem hiding this comment.
You should probably check that the last opline is the right one, i.e. compare vars of expr and result var of the last opline. Maybe this is guaranteed, but then an assert would at least be useful.
| ; (after optimizer) | ||
| ; %s:19-21 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 |
There was a problem hiding this comment.
This test already almost passes on master with the exception of C4.
There was a problem hiding this comment.
Indeed, seems this is a "recent" optimization since self is resolved to the class name as I don't think zend_optimizer_get_class_entry() is able to resolve self.
And I guess those could indeed be improved by handling unlinked classes in safe_instanceof() of dfa_pass.c
I tried doing it here as we do some other basic elision in the compiler, but also I don't really understand the optimizer ^^" but will try touching it instead. |
Girgias
force-pushed
the
elide-return-type-check-for-this-with-static
branch
from
cf008aa to
3b6fd35
Compare
|
@iluuu1994 I tried improving the Optimizer, but it still doesn't give that good results compared to doing it at compile time. Ideally one should be able to elide cases when the interface is inherited/the class is extended, but seemingly Doing it at compile time also allows traits to benefit from this optimization, covering more cases. |
| if (ce1->ce_flags & ZEND_ACC_RESOLVED_INTERFACES) { | ||
| /* Unlike the normal instanceof_function(), we have to perform a recursive | ||
| * check here, as the parent interfaces might not have been fully copied yet. */ | ||
| for (i = 0; i < ce1->num_interfaces; i++) { | ||
| if (safe_instanceof(ce1->interfaces[i], ce2, script, op_array)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure how to test we hit this case? As the tests I added don't.
2 participants
Not sure if this is the most sensible approach, I would expect this to be profitable.
Maybe we can extend this to type checks of instances of the current class so that it coversDoneselftypes that are resolved at compile time.Maybe the test should also be more somewhere more appropriate?