Optimize match(true)

[TimWolla]

Resolves#18411

[iluuu1994]

Of note: The optimizer is cautious with top-level code. Moving the entire code to a function gives:

test: ; (lines=1, args=0, vars=0, tmps=0) ; (after optimizer) ; /home/ilutov/Developer/php-src/Zend/tests/gh18411.php:3-13 0000 RETURN string("fr") 

That said, the optimizations are still lacking when $text is unknown.

Details

test: ; (lines=23, args=1, vars=2, tmps=3) ; (after optimizer) ; /home/ilutov/Developer/php-src/Zend/tests/gh18411.php:3-12 0000 CV0($text) = RECV 1 0001 T2 = FRAMELESS_ICALL_2(str_contains) CV0($text) string("Welcome") 0002 T3 = BOOL T2 0003 T2 = IS_IDENTICAL T3 bool(true) 0004 JMPNZ T2 0018 0005 T4 = FRAMELESS_ICALL_2(str_contains) CV0($text) string("Hello") 0006 T3 = BOOL T4 0007 T2 = IS_IDENTICAL T3 bool(true) 0008 JMPNZ T2 0018 0009 T4 = FRAMELESS_ICALL_2(str_contains) CV0($text) string("Bienvenue") 0010 T3 = BOOL T4 0011 T2 = IS_IDENTICAL T3 bool(true) 0012 JMPNZ T2 0020 0013 T4 = FRAMELESS_ICALL_2(str_contains) CV0($text) string("Bonjour") 0014 T3 = BOOL T4 0015 T2 = IS_IDENTICAL T3 bool(true) 0016 JMPNZ T2 0020 0017 MATCH_ERROR bool(true) 0018 T2 = QM_ASSIGN string("en") 0019 JMP 0021 0020 T2 = QM_ASSIGN string("fr") 0021 CV1($result) = QM_ASSIGN T2 0022 RETURN CV1($result) 

I wonder if this is better implemented as a DFA and SCCP pass. Namely, BOOL should be removed in DFA when the operand is already a boolean (DCE may also be an option, but it doesn't handle special cases at this point, so it may be better to keep it generic). SCCP may propagate the result for CHECK_TYPE if the inferred type matches the checked type, and may then be turned into a NOP or FREE. This may work for more use-cases.

[TimWolla]

That said, the optimizations are still lacking when $text is unknown.

I'm not sure if I follow. Is the code snippet from master or this branch? For me, my PR also successfully optimizes:

<?php $text = 'Bienvenue chez nous'; function foo($text) { $result = match (true) { !!preg_match('/Welcome/', $text), !!preg_match('/Hello/', $text) => 'en', !!preg_match('/Bienvenue/', $text), !!preg_match('/Bonjour/', $text) => 'fr', default => 'other', }; var_dump($result); } foo($text); 

Both the cast, as well as the IS_IDENTICAL disappears there, leaving only the call and JMPNZ.

[iluuu1994]

I'm not sure if I follow. Is the code snippet from master or this branch? For me, my PR also successfully optimizes:

I was referring to the behavior before your patch.

[staabm]

do we expect this patch to improve performance? if so, how much?

[edorian]

Quick test on my MacBook:

cat bench-match-true.php

<?php function foo($text) { $result = match (true) { !!preg_match('/Welcome/', $text), !!preg_match('/Hello/', $text) => 'en', !!preg_match('/Bienvenue/', $text), !!preg_match('/Bonjour/', $text) => 'fr', default => 'other', }; return $result; } $i = 1_000_000; $text = 'Bienvenue chez nous'; while($i--) { foo($text); } 

hyperfine -L version master,optimize-match-true './{version}/target/bin/php -d zend_extension=$(pwd)/{version}/modules/opcache.so -d opcache.enable_cli=1 bench-match-true.php'

Benchmark 1: ./master/target/bin/php -d zend_extension=$(pwd)/master/modules/opcache.so -d opcache.enable_cli=1 bench-match-true.php Time (mean ± σ): 121.6 ms ± 4.5 ms [User: 118.4 ms, System: 1.7 ms] Range (min … max): 112.9 ms … 132.1 ms 22 runs Benchmark 2: ./optimize-match-true/target/bin/php -d zend_extension=$(pwd)/optimize-match-true/modules/opcache.so -d opcache.enable_cli=1 bench-match-true.php Time (mean ± σ): 103.7 ms ± 4.0 ms [User: 100.6 ms, System: 1.7 ms] Range (min … max): 100.6 ms … 118.5 ms 24 runs Summary ./optimize-match-true/target/bin/php -d zend_extension=$(pwd)/optimize-match-true/modules/opcache.so -d opcache.enable_cli=1 bench-match-true.php ran 1.17 ± 0.06 times faster than ./master/target/bin/php -d zend_extension=$(pwd)/master/modules/opcache.so -d opcache.enable_cli=1 bench-match-true.php 

[dstogov]

The optimization looks right to me.

The only possible problem, is the case when the eliminated temporary variable might be used several times. (e.g. in context of ?? operator).

@iluuu1994 could you please check this.
If you don't see problems, I would support this.

[iluuu1994]

The only possible problem, is the case when the eliminated temporary variable might be used several times. (e.g. in context of ?? operator).

This would indeed be a problem for:

T2 = BOOL T1 T3 = CASE T2 ; Anything in keeps_op1_alive(), essentially. T4 = TYPE_CHECK T2 false ; After T4 = BOOL_NOT T1 T3 = CASE T2 ; T2 is no longer declared NOP 

However, I don't believe such code can currently be emitted by the compiler. CASE, CASE_STRICT, SWITCH are emitted for switch/match in a very specific pattern, e.g. multiple CASE ops in sequence, followed by a single FREE instruction for the subject. There's no way to make the terminating instruction something else like a TYPE_CHECK. Thus, I think this is correct.