-
Notifications
You must be signed in to change notification settings - Fork 587
Apply scalar context to LSLICE and ASLICE more thoroughly #23447
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
`Perl_scalar` does not explicitly handle the OP_LSLICE case and consequently some OPs are not nulled, even though the user-visible behaviour might suggest that has happened. For example, `my $x = (caller)[4,3,2]` would give the optree: ``` 2 <;> nextstate(main 2 -e:1) v:%,us,{,fea=15 ->3 a <1> padsv_store[$x:8,9] vKS/LVINTRO ->b 9 <2> lslice sK/2 ->a - <1> ex-list lK ->7 3 <0> pushmark s ->4 4 <$> const[IV 4] s ->5 5 <$> const[IV 3] s ->6 6 <$> const[IV 2] s ->7 - <1> ex-list lK ->9 7 <0> pushmark s ->8 8 <0> caller[t2] l ->9 ``` After this commit: ``` 2 <;> nextstate(main 8 -e:1) v:%,us,{,fea=15 ->3 8 <1> padsv_store[$x:8,9] vKS/LVINTRO ->9 7 <2> lslice sK/2 ->8 - <1> ex-list lK ->5 3 <0> pushmark v ->4 - <0> ex-const v ->- - <0> ex-const v ->4 4 <$> const(IV 2) s ->5 - <1> ex-list lK ->7 5 <0> pushmark s ->6 6 <0> caller[t15] l ->7 ``` The visible behaviour should be identical, but IVs 4 & 3 are no longer pushed to the stack just to be ignored.
`Perl_scalar` does not explicitly handle the OP_ASLICE case and consequently some OPs are not nulled, even though the user-visible behaviour would suggest that has happened. For example, `my $y = @x[4,3,2];` would give the optree: ``` b <;> nextstate(main 9 -e:1) v:%,us,{,fea=15 ->c i <1> padsv_store[$y:9,10] vKS/LVINTRO ->j h <@> aslice sK ->i c <0> pushmark s ->d - <1> ex-list lK ->g - <0> ex-pushmark s ->d d <$> const(IV 4) s ->e e <$> const(IV 3) s ->f f <$> const(IV 2) s ->g g <0> padav[@x:8,10] sR ->h ``` After this commit: ``` b <;> nextstate(main 9 -e:1) v:%,us,{,fea=15 ->c g <1> padsv_store[$y:9,10] vKS/LVINTRO ->h f <@> aslice sK ->g c <0> pushmark s ->d - <1> ex-list lK ->e - <0> ex-pushmark v ->- - <0> ex-const v ->- - <0> ex-const v ->d d <$> const(IV 2) s ->e e <0> padav[@x:8,10] sR ->f ``` The user-visible behaviour should be unchanged, but now only one element of @x will be removed, rather than three with two of them being immediately discarded.
Notes that the `Useless use of %s in void context` warning will apply to some constructions of array/list slices when it did not previously. This is a result of specific OP handling cases being added into Perl_scalar, in order to reduce unnecessary execution of OPs at runtime.
0676a52
to
73b58f6
Compare
This changes behaviour beyond the added warnings:
Similarly for list slice:
|
Thanks Tony. I'll dig into why. |
I've thought round this and I understand it better now. Is the following behaviour correct? (As in, it is a construct that looks similar but is not and is not expected to behave the same, or is there an inconsistency?)
For this PR, I suppose LSLICE and ASLICE could have their own scalar optimization that doesn't warn but null's any stack pushing op (e.g. CONST, PADSV) that has another of the same following it? Not entirely sure if that would be worthwhile or not.... |
That behaviour is correct, but unlike that case the indexes for a slice are in list context. You can check that in older perls by checking wantarray in f() in my example.
Another one I haven't tested (and I've cleaned my build of your branch) is how this patch handles an array instead of a function call:
|
|
I'm going to do a separate PR to include the tests suggested by Tony, then close this one down. Not sure it's worth the effort of partially optimising at this stage. |
Perl_scalar
is used to apply scalar context to OPs. This process can alsoresult in some KID OPs being nulled out. However, this has not happened
to
OP_LSLICE
andOP_ASLICE
trees to the extent that seems possible.For example, with
my $x = (1,2,3)
, the resulting optree (pre-peepholeoptimisation) is:
The interpreter only needs to execute the one
CONST
OP in scalarcontext to get the correct behaviour.
This PR adds
OP_LSLICE
andOP_ASLICE
cases to the switchstatement within
Perl_scalar
so that the same optimisationoccurs for those optrees. Previously, it did not, meaning that
unnecessary operations would be performed at runtime, with the
results then just ignored.
Note that the following seem desirable, but would need a broader
refactoring of
Perl_scalar
, and are so out of scope of this PR:CONST
OPslist
/ex-list
container and itspushmark
OP.i.e. Future work could try to get the above optree down to: