OP_AELEMFASTLEX_STORE - combined sassign/aelemfast_lex #20063
Conversation
richardleach
force-pushed
the
hydahy/OP_AELEMFASTLEX_STORE
branch
from
e497c6d to
7224a5b
Compare
richardleach
force-pushed
the
hydahy/OP_AELEMFASTLEX_STORE
branch
from
7224a5b to
3d32067
Compare
|
Rebased after #20077 merge. |
Indeed. For |
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.
richardleach
force-pushed
the
hydahy/OP_AELEMFASTLEX_STORE
branch
from
3d32067 to
f74f170
Compare
|
This shouldn't be merged until 20114 is fixed for Memoize::Once; possibly the peephole code in this PR should be amended to handle the RHS actually being the |
|
It doesn't seem like any change akin to c5bbf81 is needed here for Memoize::Once. I think this is ready to merge. |
|
@richardleach you can merge yourself no? |
Yes, I'll do it when I get a moment, assuming no-one shouts "Nooo, don't doo iiitt" between now and then. |
|
On Wed, Sep 07, 2022 at 08:28:03AM -0700, Richard Leach wrote:
> @richardleach you can merge yourself no?
Yes, I'll do it when I get a moment, assuming no-one shouts "Nooo, don't doo iiitt" between now and then.
Not so much a shout, as a delayed clearing of the throat.
Two points.
1) I don't see why you've skipped optimising negative key values. I added
support for negative keys to AELEMFAST a while back, on the grounds that
literal keys like $a[-1] etc were a lot more likely to appear in code than
$a[255]. The only change needed in pp_aelemfastlex_store() to support it
is, like in pp_aelemfast(), to do a check for key >= 0 and skip the short
cut if it's not (meaning the array fetch will be done by av_fetch()
instead of by directly accessing AvARRAY[key]). But you still get the
benefit of a single op rather than 2 ops with intermediate pushing and
popping of values.
2) I think the code which tests for and emits the "Useless assignment to a
temporary" is not needed. This condition could (although generally
shouldn't) happen when the LHS of an assignment is an arbitrary
expression, but when the LHS is the result of an array fetch, it can never
be an SvTEMP with a refcount of 1 unless something has gone horribly wrong.
Maybe replace that code with an assert that's not SvTEMP with a refcount
of 1.
Other than that, it looks good!
…--
"There's something wrong with our bloody ships today, Chatfield."
-- Admiral Beatty at the Battle of Jutland, 31st May 1916.
|
|
Thanks for the feedback, @iabyn. On:
I'll prepare a PR that addresses both suggestions. |
|
Not sure if I interpreted point 2 correctly, but |
|
|
|
On Wed, Sep 21, 2022 at 04:23:09PM -0700, Richard Leach wrote:
`assert(!(SvTEMP(targ) && SvREFCNT(targ) == 1 && !SvSMAGICAL(targ)));` doesn't get tripped by any tests, but I'm still unsure of whether or not it's safe to exchange the full `if()` condition for it. Guidance welcome!
Yeah, I forgot about tied arrays. With those, av_fetch() can return a
magical temp value which when modified, triggers a call to STORE(). But
that type of temp wouldn't trigger a warning. So asserting
! (SvTEMP(targ) && SvREFCNT(targ) == 1 && !SvMAGICAL(av))
should be ok. With a comment which explains that the assert is checking
that a "Useless assignment to a temporary" warning should never need to be
emitted here.
…--
If life gives you lemons, you'll probably develop a citric acid allergy.
|
|
Thanks. I've updated #20331 accordingly. |
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:But now will be turned into:
This is intended to be a transparent performance optimization.
It should be applicable for RHS optrees of varying complexity.
Performance:
The following unrealistic code shows the op-related speedup:
Blead:
Patched:
Note: Name is a bit unwieldy, suggestions welcome. (@leonerd - I think this PR is something different to what you had in mind as OP_PADAV_STORE?)