From be054acde5c2a6c1900573d6edeb3100bc83d965 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 | 119 +++++++++++++++++++++++++++++++++++++---------- t/perf/opcount.t | 15 +++++- 2 files changed, 108 insertions(+), 26 deletions(-) diff --git a/peep.c b/peep.c index 99c7cb6bd9f7..215c2e0cabb5 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 ); - - 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 + 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)); + + 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 dd16447bae1c..452e8e5fddad 100644 --- a/t/perf/opcount.t +++ b/t/perf/opcount.t @@ -1233,13 +1233,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