Skip to content

Commit be054ac

Browse files
richardleachrichardleach
committed
peep.c: Only remove expected empty if/else blocks structures
The expected OP tree structures for empty blocks fit for removal was: ``` +-- stub ``` or ``` +-- scope | +-- stub ``` GH #23505 showed that `{ BEGIN {} }` blocks will also generate the likes of: ``` +--scope | +--stub | +--null (ex-nextstate) ``` This commit does the minimal possible to fix this bug - the peephole optimizer now checks for the expected cases and ignores anything else. Future commits may build on this to remove this additional type of structure. Additional tests have been added now and give a useful structure for future commit tests to fit into.
1 parent 463db48 commit be054ac

File tree

2 files changed

+108
-26
lines changed

2 files changed

+108
-26
lines changed

‎peep.c‎

Lines changed: 94 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3590,41 +3590,110 @@ Perl_rpeep(pTHX_ OP *o)
35903590
case OP_COND_EXPR:
35913591
if (o->op_type == OP_COND_EXPR) {
35923592
OP *stub = cLOGOP->op_other;
3593+
OP *trueop = OpSIBLING( cLOGOP->op_first );
3594+
OP *falseop = OpSIBLING(trueop);
3595+
35933596
/* Is there an empty "if" block or ternary true branch?
35943597
If so, optimise away the OP_STUB if safe to do so. */
35953598
if (stub->op_type == OP_STUB &&
35963599
((stub->op_flags & OPf_WANT) != OPf_WANT_SCALAR)
35973600
) {
3598-
OP *trueop = OpSIBLING( cLOGOP->op_first );
3599-
3600-
assert((stub == trueop ) || (OP_TYPE_IS(trueop, OP_SCOPE) &&
3601-
((stub == cUNOPx(trueop)->op_first)) && !OpSIBLING(stub))
3602-
);
3603-
assert(!(stub->op_flags & OPf_KIDS));
3604-
3605-
cLOGOP->op_other = (stub->op_next == trueop) ?
3606-
stub->op_next->op_next :
3607-
stub->op_next;
3608-
3609-
op_sibling_splice(o, cLOGOP->op_first, 1, NULL);
3610-
3611-
if (stub != trueop) op_free(stub);
3612-
op_free(trueop);
3613-
} else
3601+
if (stub == trueop) {
3602+
/* This is very unlikely:
3603+
* cond_expr
3604+
* -condition-
3605+
* stub
3606+
* -else-
3607+
*/
3608+
assert(!(stub->op_flags & OPf_KIDS));
3609+
cLOGOP->op_other = stub->op_next;
3610+
op_sibling_splice(o, cLOGOP->op_first, 1, NULL);
3611+
op_free(stub);
3612+
break;
3613+
} else if (OP_TYPE_IS(trueop, OP_SCOPE) &&
3614+
(stub == cUNOPx(trueop)->op_first) ) {
3615+
assert(!(stub->op_flags & OPf_KIDS));
3616+
3617+
OP *stubsib = OpSIBLING(stub);
3618+
if (!stubsib) {
3619+
/* cond_expr
3620+
* -condition-
3621+
* scope
3622+
* stub
3623+
* -else-
3624+
*/
3625+
cLOGOP->op_other = trueop->op_next;
3626+
op_sibling_splice(o, cLOGOP->op_first, 1, NULL);
3627+
op_free(stub);
3628+
op_free(trueop);
3629+
break;
3630+
} else {
3631+
/* Could be something like this:
3632+
* -condition-
3633+
* scope
3634+
* stub
3635+
* null
3636+
* -else-
3637+
* But it may be more desirable (but is less
3638+
* straightforward) to transform this earlier
3639+
* in the compiler. Ignoring it for now,
3640+
* pending further exploration. */
3641+
}
3642+
}
3643+
}
36143644

36153645
/* Is there an empty "else" block or ternary false branch?
36163646
If so, optimise away the OP_STUB if safe to do so. */
36173647
stub = o->op_next;
3618-
if (stub->op_type == OP_STUB &&
3619-
((stub->op_flags & OPf_WANT) != OPf_WANT_SCALAR)
3620-
) {
3621-
assert(stub == OpSIBLING(OpSIBLING( cLOGOP->op_first )));
3622-
assert(!(stub->op_flags & OPf_KIDS));
3623-
o->op_flags |= OPf_SPECIAL; /* For B::Deparse */
3624-
o->op_next = stub->op_next;
3625-
op_sibling_splice(o, OpSIBLING(cLOGOP->op_first), 1, NULL);
3626-
op_free(stub);
3648+
if ((stub->op_flags & OPf_WANT) != OPf_WANT_SCALAR) {
3649+
if (stub->op_type == OP_STUB && !OpSIBLING(stub) ){
3650+
OP *stubsib = OpSIBLING(stub);
3651+
if ((stub == falseop) && !stubsib) {
3652+
/* cond_expr
3653+
* -condition-
3654+
* - if -
3655+
* stub
3656+
*/
3657+
assert(!(stub->op_flags & OPf_KIDS));
3658+
o->op_flags |= OPf_SPECIAL; /* For B::Deparse */
3659+
o->op_next = stub->op_next;
3660+
op_sibling_splice(o, OpSIBLING(cLOGOP->op_first), 1, NULL);
3661+
op_free(stub);
3662+
} else { /* Unexpected */ }
3663+
} else if (OP_TYPE_IS(stub,OP_ENTER) &&
3664+
OP_TYPE_IS(falseop, OP_LEAVE)) {
3665+
OP *enter = stub;
3666+
OP *stub = OpSIBLING(enter);
3667+
if (stub && OP_TYPE_IS(stub, OP_STUB) ){
3668+
assert(!(stub->op_flags & OPf_KIDS));
3669+
OP *stubsib = OpSIBLING(stub);
3670+
assert(stubsib);
3671+
if (OP_TYPE_IS(stubsib, OP_NULL) &&
3672+
!OpSIBLING(stubsib) &&
3673+
!(stubsib->op_flags & OPf_KIDS) ) {
3674+
/* cond_expr
3675+
* -condition-
3676+
* - if -
3677+
* leave
3678+
* enter
3679+
* stub
3680+
* null
3681+
*/
3682+
/* Ignoring it for now, pending further exploration.*/
3683+
/*
3684+
o->op_flags |= OPf_SPECIAL; // For B::Deparse
3685+
o->op_next = falseop->op_next;
3686+
op_sibling_splice(o, OpSIBLING(cLOGOP->op_first), 1, NULL);
3687+
op_free(enter);
3688+
op_free(stub);
3689+
op_free(stubsib);
3690+
op_free(falseop);
3691+
*/
3692+
}
3693+
}
3694+
}
36273695
}
3696+
36283697
}
36293698
/* FALLTHROUGH */
36303699
case OP_MAPWHILE:

‎t/perf/opcount.t‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1233,13 +1233,26 @@ test_opcount(0, "defined(ABC) gets constant folded",
12331233
defined => 0,
12341234
});
12351235

1236+
# Empty condop other/next branch optimizations
12361237
test_opcount(0, "Empty if{} blocks are optimised away",
1238+
sub { my $x; if ($x) { } else { 1 } },
1239+
{
1240+
stub => 0
1241+
});
1242+
1243+
test_opcount(0, "Empty else{} blocks are optimised away",
1244+
sub { my $x; if ($x) { 1 } else { } },
1245+
{
1246+
stub => 0
1247+
});
1248+
1249+
test_opcount(0, "Empty ternary true blocks are optimised away",
12371250
sub { my $x; ($x) ? () : 1 },
12381251
{
12391252
stub => 0
12401253
});
12411254

1242-
test_opcount(0, "Empty else{} blocks are optimised away",
1255+
test_opcount(0, "Empty ternary false blocks are optimised away",
12431256
sub { my $x; ($x) ? 1 : () },
12441257
{
12451258
stub => 0

0 commit comments

Comments
 (0)