Skip to content

Conversation

dstogov
Copy link
Member

@dstogov dstogov commented Mar 16, 2016

This affects only PHP VM and optimizer, and doesn't change anything else outside.

zval *var_ptr;

var_ptr = GET_OP1_ZVAL_PTR_PTR_UNDEF(BP_VAR_RW);
ZVAL_COPY_VALUE(EX_VAR(opline->result.var), var_ptr);
Copy link
Member

Choose a reason for hiding this comment

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

Since var_ptr must be IS_LONG here, so use ZVAL_LONG(EX_VAR(opline->result.var), Z_LVAL_P(var_ptr)) maybe faster.

@bwoebi
Copy link
Member

bwoebi commented Mar 16, 2016

Honestly, I preferred the old version with a single if …

Currently we have anyway a short quick branching in the ZEND_ADD opcode. This gets pretty much redundant code, also adding the risk of forgetting to update an opcode handler when changing something.

@php-pulls php-pulls merged commit 9020086 into php:master Mar 17, 2016
@laruence
Copy link
Member

hmm..... I merge this patch wrongly when I was pushing a bug fix.. how should I reopen this ?

@dstogov
Copy link
Member Author

dstogov commented Mar 17, 2016

The old version required separate opcode for each new specialization, and the whole number of opcodes is limited by 256.
Also that PoC was manually created and selector code had to reflect all changes in zend_vm_def.h
Now each specialization is defined separately, and the selector code is auto generated.

On Wed, Mar 16, 2016 at 10:29 AM -0700, "Bob Weinand" <notifications@github.commailto:notifications@github.com> wrote:

Honestly, I preferred the old version with a single if ...

Currently we have anyway a short quick branching in the ZEND_ADD opcode. This gets pretty much redundant code, also adding the risk of forgetting to update an opcode handler when changing something.

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1824#issuecomment-197443545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants