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

Opt opportunity in METAOP_ASSIGN with % and @ vars #1990

Open
zoffixznet opened this Issue Jun 28, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@zoffixznet
Contributor

zoffixznet commented Jun 28, 2018

e.g. my %h = :42foo; %h ,= :100bar on 2018.05-54-g148d7c5 still got Op (callstatic &METAOP_ASSIGN) in it, which can be rewritten to p6store.

Very likely all that needs to be done is to add a @ and % sigil branch here that sets $assignop to "p6store":

rakudo/src/Perl6/Optimizer.nqp

Lines 1916 to 1921 in 2177ee2

elsif $sigil eq '$' {
$assignop := 'assign';
}
else {
# TODO support @ and % sigils and check what else we need
# to "copy" from assign_op in Actions

It's possible the tests covering the behaviour are sparse and it might be a good idea to write a few to cover this opt.

@zoffixznet zoffixznet self-assigned this Jun 28, 2018

@zoffixznet zoffixznet removed their assignment Jun 28, 2018

dogbert17 added a commit to dogbert17/rakudo that referenced this issue Jul 2, 2018

Fix R#1990
Zoffix++, in rakudo#1990, pointed
out that there's an optimization opportunity in optimize_nameless_call
where the, faster, p6store op can be used instead when assigning
@ and % sigiled variables

@dogbert17 dogbert17 referenced this issue Jul 2, 2018

Merged

Fix R#1990 #2012

dogbert17 added a commit to dogbert17/rakudo that referenced this issue Jul 2, 2018

Fix R#1990
Zoffix++, in rakudo#1990, pointed
out that there's an optimization opportunity in optimize_nameless_call
where the, faster, p6store op can be used instead when assigning
@ and % sigiled variables
@dogbert17

This comment has been minimized.

Show comment
Hide comment
@dogbert17

dogbert17 Jul 3, 2018

Contributor

Fixed with commit 55b2e32, tests needed.

Contributor

dogbert17 commented Jul 3, 2018

Fixed with commit 55b2e32, tests needed.

@lizmat

This comment has been minimized.

Show comment
Hide comment
@lizmat

lizmat Jul 26, 2018

Contributor

Not sure how we can test this, suggestions anyone?

Contributor

lizmat commented Jul 26, 2018

Not sure how we can test this, suggestions anyone?

@zoffixznet

This comment has been minimized.

Show comment
Hide comment
@zoffixznet

zoffixznet Jul 27, 2018

Contributor

Not sure how we can test this, suggestions anyone?

The lack of tests was just a guess on my part. I don't know if there are any for the metassign for % and @ sigils. The tests should just test the metassign to such variables with some ops.

Something like this (needs to be different for array, probably, 'cause in those ,= makes a self-referential one)

my %h = :42foo, :70bar; %h ,= {:100foo, :50meow}; use Test; is-deeply %h, {:bar(70), :foo(100), :meow(50)}, ',= meta assign op with a %-sigilled var'
<camelia> rakudo-moar 08b449e1a: OUTPUT: «ok 1 - ,= meta assign op with a %-sigilled var␤»
Contributor

zoffixznet commented Jul 27, 2018

Not sure how we can test this, suggestions anyone?

The lack of tests was just a guess on my part. I don't know if there are any for the metassign for % and @ sigils. The tests should just test the metassign to such variables with some ops.

Something like this (needs to be different for array, probably, 'cause in those ,= makes a self-referential one)

my %h = :42foo, :70bar; %h ,= {:100foo, :50meow}; use Test; is-deeply %h, {:bar(70), :foo(100), :meow(50)}, ',= meta assign op with a %-sigilled var'
<camelia> rakudo-moar 08b449e1a: OUTPUT: «ok 1 - ,= meta assign op with a %-sigilled var␤»
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment