-
Notifications
You must be signed in to change notification settings - Fork 584
Implement OP_PADSV_STORE - combined sassign/padsv OP #19943
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
Conversation
993ac65
to
40d86f6
Compare
530e157
to
b631520
Compare
b631520
to
ec76523
Compare
f650f25
to
0b25d5b
Compare
0b25d5b
to
1f54c9d
Compare
1f54c9d
to
70c0312
Compare
Any further comments on this one, or does it look good to merge? |
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.
All that's here looks good, but one tiny thing is missing.
regen/op_private needs to remark that the new opcode makes use of the OPp_DEFER
bits. Add it to the list around line 416.
Thanks for spotting that. I'll take a look later today. Might not be straightforward - regen complains that: (I can't remember offhand if B::Deparse needs the |
70c0312
to
87c9ebc
Compare
Assuming all CI tests pass, OP_PADSV_CONST doesn't seem to need the |
87c9ebc
to
a4da112
Compare
After working on #20036 and looking a bit more at this PR, I realised that the basic optimization will not need to support Note: It also seemed simple enough to add support for assigning simple Hopefully, UNOPs (and possibly other op types) will soon get an |
a4da112
to
0b993ae
Compare
0b993ae
to
066cb27
Compare
This commit introduces a new OP to replace simple cases of OP_SASSIGN and OP_AELEMFAST_LEX. (Similar concept to GH Perl#19943) For example, `my @ary; $ary[0] = "boo"` is currently implemented as: 7 <2> sassign vKS/2 ->8 5 <$> const[PV "boo"] s ->6 - <1> ex-aelem sKRM*/2 ->7 6 <0> aelemfast_lex[@ary:1,2] sRM ->7 - <0> ex-const s ->- But now will be turned into: 6 <1> aelemfastlex_store[@ary:1,2] vKS ->7 5 <$> const(PV "boo") s ->6 - <1> ex-aelem sKRM*/2 ->6 - <0> ex-aelemfast_lex sRM ->6 - <0> ex-const s ->- This is intended to be a transparent performance optimization. It should be applicable for RHS optrees of varying complexity.
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.
One tiny whitespace issue but otherwise this all looks good now.
Will you be writing a perldelta.pod entry?
t/perf/opcount.t
Outdated
padsv => 1, | ||
padsv_store => 3, | ||
undef => 1, |
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.
Tiny whitespace alignment issue here. Make the longest one have a single space, and align the others at the =>
to match.
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.
Thanks. I took out the undef
assignment, since #20077 will change it, and aligned the spaces on the remainder.
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.
Re: perldelta.pod, an entry per optimization seems like too much. Would a placeholder that covers multiple (and could link to individual PRs or commits) suffice? Something like:
"Multiple optree optimizations have been applied in order to speed up common operations."
This commit introduces a new OP to replace simple cases of OP_SASSIGN and OP_PADSV. For example, 'my $x = 1' is currently implemented as: 1 <;> nextstate(main 1 -e:1) v:{ 2 <$> const(IV 1) s 3 <0> padsv[$x:1,2] sRM*/LVINTRO 4 <2> sassign vKS/2 But now will be turned into: 1 <;> nextstate(main 1 -e:1) v:{ 2 <$> const(IV 1) s 3 <1> padsv_store[$x:1,2] vKMS/LVINTRO This intended to be a transparent performance optimization. It should be applicable for RHS optrees of varying complexity.
066cb27
to
61c4fed
Compare
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.
Excellent, looking good now.
This can be merged assuming CI is happy.
This commit introduces a new OP to replace simple cases of OP_SASSIGN and OP_AELEMFAST_LEX. (Similar concept to GH Perl#19943) For example, `my @ary; $ary[0] = "boo"` is currently implemented as: 7 <2> sassign vKS/2 ->8 5 <$> const[PV "boo"] s ->6 - <1> ex-aelem sKRM*/2 ->7 6 <0> aelemfast_lex[@ary:1,2] sRM ->7 - <0> ex-const s ->- But now will be turned into: 6 <1> aelemfastlex_store[@ary:1,2] vKS ->7 5 <$> const(PV "boo") s ->6 - <1> ex-aelem sKRM*/2 ->6 - <0> ex-aelemfast_lex sRM ->6 - <0> ex-const s ->- This is intended to be a transparent performance optimization. It should be applicable for RHS optrees of varying complexity.
This commit introduces a new OP to replace simple cases of OP_SASSIGN and OP_AELEMFAST_LEX. (Similar concept to GH Perl#19943) For example, `my @ary; $ary[0] = "boo"` is currently implemented as: 7 <2> sassign vKS/2 ->8 5 <$> const[PV "boo"] s ->6 - <1> ex-aelem sKRM*/2 ->7 6 <0> aelemfast_lex[@ary:1,2] sRM ->7 - <0> ex-const s ->- But now will be turned into: 6 <1> aelemfastlex_store[@ary:1,2] vKS ->7 5 <$> const(PV "boo") s ->6 - <1> ex-aelem sKRM*/2 ->6 - <0> ex-aelemfast_lex sRM ->6 - <0> ex-const s ->- This is intended to be a transparent performance optimization. It should be applicable for RHS optrees of varying complexity.
This commit introduces a new OP to replace simple cases of OP_SASSIGN and OP_AELEMFAST_LEX. (Similar concept to GH Perl#19943) For example, `my @ary; $ary[0] = "boo"` is currently implemented as: 7 <2> sassign vKS/2 ->8 5 <$> const[PV "boo"] s ->6 - <1> ex-aelem sKRM*/2 ->7 6 <0> aelemfast_lex[@ary:1,2] sRM ->7 - <0> ex-const s ->- But now will be turned into: 6 <1> aelemfastlex_store[@ary:1,2] vKS ->7 5 <$> const(PV "boo") s ->6 - <1> ex-aelem sKRM*/2 ->6 - <0> ex-aelemfast_lex sRM ->6 - <0> ex-const s ->- This is intended to be a transparent performance optimization. It should be applicable for RHS optrees of varying complexity.
This commit introduces a new OP to replace simple cases of OP_SASSIGN and OP_AELEMFAST_LEX. (Similar concept to GH #19943) For example, `my @ary; $ary[0] = "boo"` is currently implemented as: 7 <2> sassign vKS/2 ->8 5 <$> const[PV "boo"] s ->6 - <1> ex-aelem sKRM*/2 ->7 6 <0> aelemfast_lex[@ary:1,2] sRM ->7 - <0> ex-const s ->- But now will be turned into: 6 <1> aelemfastlex_store[@ary:1,2] vKS ->7 5 <$> const(PV "boo") s ->6 - <1> ex-aelem sKRM*/2 ->6 - <0> ex-aelemfast_lex sRM ->6 - <0> ex-const s ->- This is intended to be a transparent performance optimization. It should be applicable for RHS optrees of varying complexity.
This commit introduces a new OP to replace simple cases of OP_SASSIGN and OP_AELEMFAST_LEX. (Similar concept to GH Perl#19943) For example, `my @ary; $ary[0] = "boo"` is currently implemented as: 7 <2> sassign vKS/2 ->8 5 <$> const[PV "boo"] s ->6 - <1> ex-aelem sKRM*/2 ->7 6 <0> aelemfast_lex[@ary:1,2] sRM ->7 - <0> ex-const s ->- But now will be turned into: 6 <1> aelemfastlex_store[@ary:1,2] vKS ->7 5 <$> const(PV "boo") s ->6 - <1> ex-aelem sKRM*/2 ->6 - <0> ex-aelemfast_lex sRM ->6 - <0> ex-const s ->- This is intended to be a transparent performance optimization. It should be applicable for RHS optrees of varying complexity.
This commit introduces a new OP to replace simple cases
of OP_SASSIGN and OP_PADSV.
For example,
my $x = 1
is currently implemented as:But now will be turned into:
This intended to be a transparent performance optimization.
It should be applicable for RHS optrees of varying complexity.
Notes:
Performance:
The following unrealistic code shows the op-related speedup:
Blead:
Patched:
The extent to which users will notice a benefit in real code will depend on things like how often the optimized op is used and what path is taken through
Perl_sv_setsv
.