@nikic I'm not that amazed at the runtime checking of liveness ranges; is it not possible to simply store two live ranges for DUP_TMP? In the example above for V5 from L6 to L12 and from L16 to L17?

Also, shouldn't it be named COPY_TMP? I associate DUP with ZVAL_DUP, which actually duplicates arrays. And COPY is for refcount only operations.

I'm not that amazed at the runtime checking of liveness ranges; is it not possible to simply store two live ranges for DUP_TMP? In the example above for V5 from L6 to L12 and from L16 to L17?

That's an interesting idea. I suspect that we have assumptions that the opcode above the start of the live range defines the variable. But if this can be made to work then I would prefer it as well.

Also, shouldn't it be named COPY_TMP? I associate DUP with ZVAL_DUP, which actually duplicates arrays. And COPY is for refcount only operations.

Yeah, makes sense.

I suspect that we have assumptions that the opcode above the start of the live range defines the variable.

Not at runtime at least for ZEND_LIVE_TMP. I don't know about opcache though.

Not at runtime at least for ZEND_LIVE_TMP. I don't know about opcache though.

Opcache has this assumption at least here: https://github.com/php/php-src/blob/master/ext/opcache/Optimizer/dce.c#L517 We could check for the case where the defining opline is a jump and assume it's a split range in that case.

@dstogov Do you have any opinion on this?

I would prefer to reject the idea, if it can't be implemented in a simple way.

What was wrong with original implementation https://github.com/php/php-src/pull/1795/files ?
Only notices about uninitialized variables?

May be we may suppress them using BP_VAR_W instead of BP_VAR_RW, or BP_VAR_RW_IS or BP_VAR_RW + some additional flag.

@dstogov The problem in #1795 is that it always evaluated the right hand side. Part of the purpose of ??= is that you can write something like $a->b ??= new Obj and new Obj will only be executed when it's actually needed.

@nikic Got it.

Another idea - JMP_IF_NOT_NULL opcode that keeps source operand alive.

L0 (2): INIT_FCALL_BY_NAME 0 string("foo")
L1 (2): V2 = DO_FCALL_BY_NAME
L2 (2): INIT_FCALL_BY_NAME 0 string("bar")
L3 (2): V3 = DO_FCALL_BY_NAME
L4 (2): V1 = FETCH_DIM_W CV0($a) V2 ; may be need special fetch mode
L5 (2): V2 = FETCH_DIM_W V1 V3 ; may be need special fetch mode
L6 (2): JMP_IF_NOT_NULL V2 L10 ; also keeps V2 alive
L7 (2): INIT_FCALL_BY_NAME 0 string("baz")
L8 (2): V1 = DO_FCALL_BY_NAME
L9 (2): ASSIGN V2, V1
L10:

Unfortunately, this won't work with overloaded arrays and objects, but I'm not sure about their support in your implementation, as well.

In general, this may be solved, introducing JMP_IF_NOT_NULL_DIM and others, that keep both arguments alive.

@dstogov This would furthermore require JMP_IF_NOT_NULL to convert V2 into a reference, otherwise it may be invalidated prior to the ASSIGN.

@nikic you are right (INDIRECT pointers may become dangled).

Then, I don't see an efficient implementation at all.
I think, complication of the engine (live ranges with holes) doesn't cost this feature.

@dstogov The engine itself doesn't get significantly more complicated if we just have multiple single live ranges for the same var. Just opcache needs handling of that...

I have pushed a new version implementing @bwoebi's suggestion to represent this as two live-ranges.

I'm not sure if this is better or not. I think ideally opcache would not deal with live-ranges and instead fully recompute them after optimization. But special cases like this make this hard.

@nikic I'm wondering whether it's possible to emit live ranges as a separate step after compilation?
Tracking the emitting of used tmp/vars and following that through jumps.

We just need to maintain a list of non-consuming and jumping opcode operands then.

Is there actually any reason / benefit why we are squeezing their generation into the main compilation step?
As you say, if it were its own step, opcache could just benefit from that function directly and simply regenerate it.

@nikic @bwoebi may be recomputing line ranges at once is a good idea (not sure about interactive mode). This may even simplify things.

@dstogov interactive mode is on a block-boundary. So should not be problematic here as we have no live ranges existing across a block (with block I mean like foreach, switch, ...).

I'll give the separate live range computation a try (separately from this feature).

@nikic great. ping me about result.

Rebased over liveness changes. What I ended up doing here is add special handling for COPY_TMP during live range calculation and emit two live ranges for this case. It's not pretty, but I think this is okay as long as COPY_TMP is the only opcode with the live range splitting problem it's better to handle it as a special case than make the overall algorithm more complicated.

Looks good to me, thanks :-)

@nikic Unfortunately, I broke this PR by recent optimization commits, but form source review, I don't see any serious problems now.

Updated for the live range optimizations and merged as a50198d.

ossfuzz has discovered a problem with conjunction with assert(). The following PHP code is compiled incorrectly.

<?php
assert(y)[y] ??= y;

$_main:
     ; (lines=26, args=0, vars=0, tmps=10)
     ; (before optimizer)
     ; /home/dmitry/tmp/fuzz-60129.php:1-3
     ; return  [] RANGE[0..0]
0000 V2 = ASSERT_CHECK 0007
0001 INIT_FCALL 2 112 string("assert")
0002 T0 = FETCH_CONSTANT string("y")
0003 T1 = COPY_TMP T0
0004 SEND_VAL T0 1
0005 SEND_VAL string("assert(y)") 2
0006 V2 = DO_ICALL
0007 T3 = FETCH_CONSTANT string("y")
0008 T4 = COPY_TMP T3
0009 T5 = FETCH_DIM_IS V2 T3
0010 T6 = COALESCE T5 0022
0011 T7 = FETCH_CONSTANT string("y")
0012 V8 = ASSERT_CHECK 0017
0013 INIT_FCALL 2 112 string("assert")
0014 SEND_VAL T1 1                                                 ; <== T1 may be uninitialized here
0015 SEND_VAL string("assert(y)") 2
0016 V8 = DO_ICALL
0017 V8 = SEPARATE V8
0018 T9 = ASSIGN_DIM V8 T4
0019 OP_DATA T7
0020 T6 = QM_ASSIGN T9
0021 JMP 0024
0022 FREE T1
0023 FREE T4
0024 FREE T6
0025 RETURN int(1)

CC @iluuu1994