Skip to content

Commit 73c046c

Browse files
richardleachkhwilliamson
richardleach
authored and
khwilliamson
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 7cdc183 commit 73c046c

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
@@ -1290,13 +1290,26 @@ test_opcount(0, "defined(ABC) gets constant folded",
12901290
defined => 0,
12911291
});
12921292

1293+
# Empty condop other/next branch optimizations
12931294
test_opcount(0, "Empty if{} blocks are optimised away",
1295+
sub { my $x; if ($x) { } else { 1 } },
1296+
{
1297+
stub => 0
1298+
});
1299+
1300+
test_opcount(0, "Empty else{} blocks are optimised away",
1301+
sub { my $x; if ($x) { 1 } else { } },
1302+
{
1303+
stub => 0
1304+
});
1305+
1306+
test_opcount(0, "Empty ternary true blocks are optimised away",
12941307
sub { my $x; ($x) ? () : 1 },
12951308
{
12961309
stub => 0
12971310
});
12981311

1299-
test_opcount(0, "Empty else{} blocks are optimised away",
1312+
test_opcount(0, "Empty ternary false blocks are optimised away",
13001313
sub { my $x; ($x) ? 1 : () },
13011314
{
13021315
stub => 0

0 commit comments

Comments
 (0)