Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pass opline as argument to opcode handlers in CALL VM #17952
base: master
Are you sure you want to change the base?
Pass opline as argument to opcode handlers in CALL VM #17952
Changes from all commits
04bff56
1f12382
6ebf91b
a773fbe
5a63362
919e1a0
dc1a571
46de8b2
841f0ea
cb83679
f382249
fad6772
5ae328d
44f0a90
0da3e5f
a5b9f66
e00982e
42f60ba
64f27cc
91284db
93317c0
f4f9ec5
54cd812
ad3c920
f013aa1
d50c5d1
8637220
ced459a
23a1cb4
d31339f
0bf80cf
5d3af10
833511c
b807d6f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I didn't like this higher half stuff because it adds more complexity, but I'm glad to see it doesn't make an improvement so this can be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes
EX(opline) = opline
whenZEND_VM_IP_GLOBAL_REG
is defined.This may cause output of incorrect line number in error message.
May be I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's correct.
This causes
opline
to be saved toEX(opline)
in the CALL VM (and in the HYBRID VM, as before). Saving used to be unnecessary asEX(opline)
was always up to date in the CALL VM. I believe that the#ifdef
is redundant asSAVE_OPLINE()
was a no-op on the CALL VM.Large diffs are not rendered by default.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in ext/opcache/jit/ir will be submitted as a proper PR on IR repository.
Here I add an optimization so that
is compiled to a single
bts
instruction wheny
is a large power of two. This avoids copyingy
to a register first (becauseor
doesn't accept an imm64 op2), saving an instruction and reducing code size.So this optimizes this:
Into this:
GCC does this optimization: https://godbolt.org/z/ovEWGPr3r (but not Clang).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, but please name the rule
BTS
. Even better implement the similar optimization forBTR
and use the common rule( e.g. BIT_OP) . I would glad to accept this for IR independently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a PR here: dstogov/ir#111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops. It looks like the previous prototype was wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we redifine
ZEND_VM_ENTER_BIT
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zend_jit_internal.h
has its own definition of some VM macros.Here I define
ZEND_VM_ENTER_BIT
twice because of #17952 (comment). The non-ZEND_HIGH_HALF_KERNEL case is faster and more portable, so I'm going to keep only#define ZEND_VM_ENTER_BIT 1ULL
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also possible to "teach" IR testing the high bit checking the sign (I can do this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do it, I would be happy to check how it affects performances