[RFC] Match expression by iluuu1994 · Pull Request #5371 · php/php-src · GitHub
source link: https://github.com/php/php-src/pull/5371
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[RFC] Match expression #5371
Conversation
Member
carusogabriel left a comment
If I want to write |
@carusogabriel Yep. Normal whitespace rules apply. |
Contributor
TysonAndre commented on Apr 13
Because there are opcache bugs, that'll need to be addressed, almost definitely by introducing a new opcode such as On a php-src build with this PR (and a few other proposed changes) <?php // Observed: // With opcache.enable_cli=0, this correctly throws an UnhandledMatchError // With opcache.enable_cli=1, this incorrectly throws a RuntimeException function test() { $x = '2'; match($x) { 1, 2, 3, 4, 5 => { throw new RuntimeException(); }, }; } test(); Opcache sees that '2' and 2 are weakly equivalent, and assumes that optimizing away the other cases of the ZEND_SWITCH_LONG is safe (command used:
|
Contributor
TysonAndre commented on Apr 13
Another optimization I'd wanted to see in opcache for a while, but is out of the scope of this PR would be to automatically convert
(Opcache already does a similar optimization for non-strict in_array) |
Thank you @TysonAndre for your very thorough review! |
Contributor
Author
iluuu1994 left a comment
is the cause of the incorrect optimization. Replacing it with this (the default case) solves the problem: for (s = 0; s < block->successors_count; s++) { scdf_mark_edge_feasible(scdf, block_num, block->successors[s]); } return; I'll have to investigate further tomorrow. |
Contributor
TysonAndre commented on Apr 14
Also, there'd be a merge conflict if both this and the "throw expression" RFC pass. Passing -void zend_compile_throw(zend_ast *ast) /* {{{ */ +void zend_compile_throw(znode *result, zend_ast *ast) /* {{{ */ { zend_ast *expr_ast = ast->child[0]; @@ -4565,6 +4565,11 @@ void zend_compile_throw(zend_ast *ast) /* {{{ */ zend_compile_expr(&expr_node, expr_ast); zend_emit_op(NULL, ZEND_THROW, &expr_node, NULL); + + if (result != NULL) { + result->op_type = IS_CONST; + ZVAL_BOOL(&result->u.constant, 1); + } } /* }}} */ @@ -5378,7 +5383,7 @@ void zend_compile_match(znode *result, zend_ast *ast) /* {{{ */ zend_ast *error_args_ast = zend_ast_create_list(0, ZEND_AST_ARG_LIST); zend_ast *new_error_ast = zend_ast_create(ZEND_AST_NEW, error_name_ast, error_args_ast); zend_ast *throw_ast = zend_ast_create(ZEND_AST_THROW, new_error_ast); - zend_compile_throw(throw_ast); + zend_compile_throw(NULL, throw_ast); |
ext/opcache/Optimizer/dfa_pass.c
Outdated
@@ -925,6 +928,7 @@ static int zend_dfa_optimize_jmps(zend_op_array *op_array, zend_ssa *ssa) | ||
} |
||
break; |
||
case ZEND_SWITCH_STRING: |
||
case ZEND_MATCH_STRING: |
||
if (opline->op1_type == IS_CONST) { |
||
zval *zv = CT_CONSTANT_EX(op_array, opline->op1.constant); |
||
if (Z_TYPE_P(zv) != IS_STRING) { |
TysonAndre on Apr 18 •
Contributor
I don't see any obvious bugs.
For this branch in particular, I think that if something is a ZEND_SWITCH_STRING, it would have to go to the first case (case by case IS_EQUAL comparisons), but if something was a ZEND_MATCH_STRING, it could safely go to the last case (the default case) instead?
(Both are correct, but the alternative is more efficient and I think it should eliminate more dead code)
removed_ops++; MAKE_NOP(opline); opline->extended_value = 0; take_successor_ex(ssa, block_num, block, block->successors[0]); // take the last successor instead?
iluuu1994 on Apr 20
Author
Contributor
Unfortunately the following doesn't get optimized:
function test1(string $value) { echo match($value) { 1, 2, 3, 4, 5 => 'a', default => 'b', }; } test1(1);
0000 CV0($value) = RECV 1
0001 MATCH_LONG CV0($value) 1: 0002, 2: 0002, 3: 0002, 4: 0002, 5: 0002, default: 0004
0002 T1 = QM_ASSIGN string("a")
0003 JMP 0005
0004 T1 = QM_ASSIGN string("b")
0005 ECHO T1
0006 RETURN null
But that has more to do with the fact that $value
is a CV
and not a CONST
.
ext/opcache/Optimizer/dfa_pass.c
Outdated
@@ -896,6 +898,7 @@ static int zend_dfa_optimize_jmps(zend_op_array *op_array, zend_ssa *ssa) | ||
break; |
||
} |
||
case ZEND_SWITCH_LONG: |
||
case ZEND_MATCH_LONG: |
||
if (opline->op1_type == IS_CONST) { |
||
zval *zv = CT_CONSTANT_EX(op_array, opline->op1.constant); |
||
if (Z_TYPE_P(zv) != IS_LONG) { |
TysonAndre on Apr 18
Contributor
Same here - for ZEND_MATCH_LONG, I'd assume taking the last successor instead of the first successor would be more efficient.
I'm not familiar with what take_successor_ex is actually doing though. I'd hope that'd work, but it may end up being more complicated than that.
take_successor_ex(ssa, block_num, block, block->successors[0]); // take last successor instead
Thanks @TysonAndre, your comments are tremendously helpful! Keep them coming |
Contributor
TysonAndre commented on Apr 18
I'm not sure why 017.phpt and 019.phpt started failing in travis, possibly adding to a hash table or looking up in a hash table the wrong way on some platforms, or ma
|
Contributor
TysonAndre commented on Apr 18
And the JIT tests are failing, probably because the MATCH_STRING and MATCH_LONG opcodes would need to be updated for MATCH_STRING and MATCH_LONG) (e.g. it would work when there was an IS_IDENTICAL check to fall through to, but not when they get eliminated as dead code) https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=6794&view=ms.vss-test-web.build-test-results-tab&runId=155108&resultId=103028&paneView=debug Unfortunately, there are few people familiar with the JIT, and I'm not one of them. I'm terrible at assembly, but it looks like in ext/opcache/jit/zend_jit_x86.dasc, returning 1 in zend_jit_switch and adding a todo might be an option for I can't read what it's doing, but from the output of tests, it looks like it'd be emitting assembly to fall through to the next opcode in some cases, which works for If zend_jit_switch returned 1, it looks like opcache would just be prevented from JITing functions containing e.g. 013.phpt
|
Member
nikic left a comment
LG. Please squash when merging, and add a note to the UPGRADING file. |
Finally Thanks again @TysonAndre and @nikic for all the reviews! |
Unfortunately we have some exception handling issues, found by msan:
Note that T1 range goes from 3-5, including MATCH_ERROR, which means that we'll try to free T1 when match throws, even though it has not been initialized. |
Contributor
TysonAndre commented on Jul 11
@nikic - What about moving the I.e. instead of this:
Use the following instead?
I'm assuming that the solution you proposed above (T1 = MATCH_ERROR) works, it's just unoptimized, and that tooling for php 8.0 can assume that |
@nikic I'm assuming you had the same misoptimization? Specifically, this is wrong: - 0045 ECHO string("1") + 0045 ECHO T1 ... - 0055 ECHO string("1, 1") + 0055 ECHO string("2, 1") The exact place it is happening is here (for
Not sure if this is a match specific issue or a general optimization bug. @TysonAndre Thanks for your suggestion, I'll try it! |
Scratch that. I suspect that this feature isn't yet available in alpha2 which I was using for testing. Sorry for the noise. |
@iluuu1994 Thanks for confirming. I look forward to a next alpha (for Windows), so I can actually test it. |
You can use my PHP 8.0 docker image (on linux/mac or WSL2): |
First: I know this is not support forum so I am sorry about my question. Second; I read on RFC that Real-life example; I have base class that is being extended with 2 different classes (will be more) and each child must have matching DTO: public static function createFromEntity(BaseConfig $config): self { if ($config instanceof ServiceCategoryConfig) { return new ServiceCategoryConfigDTO($config->getCategory()); } if ($config instanceof DiscountConfig) { return new DiscountConfigDTO($config->getDiscount()); } // do something else here } Can If it can't, what about not requiring param and be something like: return match() { $config instance of ServiceCategoryConfig => new ServiceCategoryConfigDTO($config->getCategory()), $config instance of DiscountConfig => new DiscountConfigDTO($config->getDiscount()), default => // do something else here } |
@zmitic Yes, but as with |
@iluuu1994 Thank you, this is truly amazing feature! |
spiritinlife commented on Oct 2 •
With the addition of union types wouldn't it make sense for
Or even better something like this
|
@spiritinlife Matching by type would be part of pattern matching which is being discussed for 8.1. The syntax is not clear yet. |
@iluuu1994 I see thank you, is there a discussion i can follow for this ? |
@spiritinlife No not at the moment. We're working on an RFC for enums/ADTs which would also make use of pattern matching. But nothing specific for type pattern matching. |
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK