Skip to content
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

Add type inference for various missing opcodes #13304

Closed

Conversation

iluuu1994
Copy link
Member

No description provided.

@iluuu1994 iluuu1994 force-pushed the missing-opcode-type-inference branch from 035d703 to 514743d Compare February 1, 2024 17:29
Comment on lines +3996 to +3999
case ZEND_SEPARATE:
UPDATE_SSA_TYPE(t1, ssa_op->result_def);
COPY_SSA_OBJ_TYPE(ssa_op->op1_use, ssa_op->result_def);
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't properly support the SEPARATE yet, it may make sense to remove it and commit everything else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, let's do it that way for now. SEPARATE shouldn't be used much in practice anyway.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good, but the inference for SEPARATE may work improperly, because of missing inferences for IS_VAR.

I would suggest to commit this without SEPEARATE.
Please monitor the possible related failures in the "nightly" tests.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Feb 5, 2024

Here's the problematic case for SEAPRATE, for future reference.

--TEST--
Trying to write on method return
--FILE--
<?php

function d() {
    return null;
}

function test() {
    d()[] = 2;
    d()[] = 3;
}

for ($i = 0; $i < 10; $i++) {
    test();
}

?>
===DONE===
--EXPECT--
===DONE===

In short, the JIT omits a Z_TYPE_INFO(EX_CV(0)) = IS_NULL; for d()[] = 3; because it incorrectly deduces that the VAR already contains this type. It misses the fact that ASSIGN_DIM may change the type for non-reference VARs.

@iluuu1994 iluuu1994 closed this in 79e8f20 Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants