From 73c046c733ed76aa41faecfb7f089ad8cc553a4d Mon Sep 17 00:00:00 2001 From: Richard Leach Date: Wed, 30 Jul 2025 21:16:15 +0000 Subject: [PATCH] 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. --- peep.c | 117 +++++++++++++++++++++++++++++++++++++---------- t/perf/opcount.t | 15 +++++- 2 files changed, 107 insertions(+), 25 deletions(-) diff --git a/peep.c b/peep.c index 517037cff0..11ce7e712d 100644 --- a/peep.c +++ b/peep.c @@ -3590,41 +3590,110 @@ Perl_rpeep(pTHX_ OP *o) case OP_COND_EXPR: if (o->op_type == OP_COND_EXPR) { OP *stub = cLOGOP->op_other; + OP *trueop = OpSIBLING( cLOGOP->op_first ); + OP *falseop = OpSIBLING(trueop); + /* Is there an empty "if" block or ternary true branch? If so, optimise away the OP_STUB if safe to do so. */ if (stub->op_type == OP_STUB && ((stub->op_flags & OPf_WANT) != OPf_WANT_SCALAR) ) { - OP *trueop = OpSIBLING( cLOGOP->op_first ); + if (stub == trueop) { + /* This is very unlikely: + * cond_expr + * -condition- + * stub + * -else- + */ + assert(!(stub->op_flags & OPf_KIDS)); + cLOGOP->op_other = stub->op_next; + op_sibling_splice(o, cLOGOP->op_first, 1, NULL); + op_free(stub); + break; + } else if (OP_TYPE_IS(trueop, OP_SCOPE) && + (stub == cUNOPx(trueop)->op_first) ) { + assert(!(stub->op_flags & OPf_KIDS)); - assert((stub == trueop ) || (OP_TYPE_IS(trueop, OP_SCOPE) && - ((stub == cUNOPx(trueop)->op_first)) && !OpSIBLING(stub)) - ); - assert(!(stub->op_flags & OPf_KIDS)); - - cLOGOP->op_other = (stub->op_next == trueop) ? - stub->op_next->op_next : - stub->op_next; - - op_sibling_splice(o, cLOGOP->op_first, 1, NULL); - - if (stub != trueop) op_free(stub); - op_free(trueop); - } else + OP *stubsib = OpSIBLING(stub); + if (!stubsib) { + /* cond_expr + * -condition- + * scope + * stub + * -else- + */ + cLOGOP->op_other = trueop->op_next; + op_sibling_splice(o, cLOGOP->op_first, 1, NULL); + op_free(stub); + op_free(trueop); + break; + } else { + /* Could be something like this: + * -condition- + * scope + * stub + * null + * -else- + * But it may be more desirable (but is less + * straightforward) to transform this earlier + * in the compiler. Ignoring it for now, + * pending further exploration. */ + } + } + } /* Is there an empty "else" block or ternary false branch? If so, optimise away the OP_STUB if safe to do so. */ stub = o->op_next; - if (stub->op_type == OP_STUB && - ((stub->op_flags & OPf_WANT) != OPf_WANT_SCALAR) - ) { - assert(stub == OpSIBLING(OpSIBLING( cLOGOP->op_first ))); - assert(!(stub->op_flags & OPf_KIDS)); - o->op_flags |= OPf_SPECIAL; /* For B::Deparse */ - o->op_next = stub->op_next; - op_sibling_splice(o, OpSIBLING(cLOGOP->op_first), 1, NULL); - op_free(stub); + if ((stub->op_flags & OPf_WANT) != OPf_WANT_SCALAR) { + if (stub->op_type == OP_STUB && !OpSIBLING(stub) ){ + OP *stubsib = OpSIBLING(stub); + if ((stub == falseop) && !stubsib) { + /* cond_expr + * -condition- + * - if - + * stub + */ + assert(!(stub->op_flags & OPf_KIDS)); + o->op_flags |= OPf_SPECIAL; /* For B::Deparse */ + o->op_next = stub->op_next; + op_sibling_splice(o, OpSIBLING(cLOGOP->op_first), 1, NULL); + op_free(stub); + } else { /* Unexpected */ } + } else if (OP_TYPE_IS(stub,OP_ENTER) && + OP_TYPE_IS(falseop, OP_LEAVE)) { + OP *enter = stub; + OP *stub = OpSIBLING(enter); + if (stub && OP_TYPE_IS(stub, OP_STUB) ){ + assert(!(stub->op_flags & OPf_KIDS)); + OP *stubsib = OpSIBLING(stub); + assert(stubsib); + if (OP_TYPE_IS(stubsib, OP_NULL) && + !OpSIBLING(stubsib) && + !(stubsib->op_flags & OPf_KIDS) ) { + /* cond_expr + * -condition- + * - if - + * leave + * enter + * stub + * null + */ + /* Ignoring it for now, pending further exploration.*/ + /* + o->op_flags |= OPf_SPECIAL; // For B::Deparse + o->op_next = falseop->op_next; + op_sibling_splice(o, OpSIBLING(cLOGOP->op_first), 1, NULL); + op_free(enter); + op_free(stub); + op_free(stubsib); + op_free(falseop); + */ + } + } + } } + } /* FALLTHROUGH */ case OP_MAPWHILE: diff --git a/t/perf/opcount.t b/t/perf/opcount.t index 469d4fc92b..8e8af307bd 100644 --- a/t/perf/opcount.t +++ b/t/perf/opcount.t @@ -1290,13 +1290,26 @@ test_opcount(0, "defined(ABC) gets constant folded", defined => 0, }); +# Empty condop other/next branch optimizations test_opcount(0, "Empty if{} blocks are optimised away", + sub { my $x; if ($x) { } else { 1 } }, + { + stub => 0 + }); + +test_opcount(0, "Empty else{} blocks are optimised away", + sub { my $x; if ($x) { 1 } else { } }, + { + stub => 0 + }); + +test_opcount(0, "Empty ternary true blocks are optimised away", sub { my $x; ($x) ? () : 1 }, { stub => 0 }); -test_opcount(0, "Empty else{} blocks are optimised away", +test_opcount(0, "Empty ternary false blocks are optimised away", sub { my $x; ($x) ? 1 : () }, { stub => 0