Optimise away empty else {} blocks

[richardleach]

Description
@rjbs suggested on #p5p:

snowdrop:~$ perl -MO=Concise -E 'if ($x) { say 1 }'
8  <@> leave[1 ref] vKP/REFC ->(end)
1     <0> enter v ->2
2     <;> nextstate(main 2 -e:1) v:%,us,{,fea=15 ->3
-     <1> null vK/1 ->8
4        <|> and(other->5) vK/1 ->8
-           <1> ex-rv2sv sK/1 ->4
3              <$> gvsv(*x) s ->4
-           <@> scope vK ->-
-              <;> ex-nextstate(main 4 -e:1) v:%,us,fea=15 ->5
7              <@> say vK ->8
5                 <0> pushmark s ->6
6                 <$> const(IV 1) s ->7
-e syntax OK
snowdrop:~$ perl -MO=Concise -E 'if ($x) { say 1 } else {} '
8  <@> leave[1 ref] vKP/REFC ->(end)
1     <0> enter v ->2
2     <;> nextstate(main 2 -e:1) v:%,us,{,fea=15 ->3
-     <1> null vK/1 ->8
4        <|> cond_expr(other->5) vK/1 ->9
-           <1> ex-rv2sv sK/1 ->4
3              <$> gvsv(*x) s ->4
-           <@> scope vK ->-
-              <;> ex-nextstate(main 4 -e:1) v:%,us,fea=15 ->5
7              <@> say vK ->8
5                 <0> pushmark s ->6
6                 <$> const(IV 1) s ->7
b           <@> leave vK ->8
9              <0> enter v ->a
a              <0> stub vP ->b

"I bet the subtree starting at (b) in the second dump could be optimized away. Then you could always put "... } else { (comment only) }" at the end of an if/elsif block."

[richardleach]

The following experiment essentially results in the second example generating the first optree, not the second. Not 100% sure if that's the best approach, and even if it is, the patch is incomplete and needs work.

diff --git a/op.c b/op.c
index 81073cc737..f63c0ac0eb 100644
--- a/op.c
+++ b/op.c
@@ -4487,6 +4487,9 @@ Perl_op_scope(pTHX_ OP *o)
 {
     if (o) {
         if (o->op_flags & OPf_PARENS || PERLDB_NOOPT || TAINTING_get) {
+            if (OP_TYPE_IS(o, OP_STUB))
+                return o;
+
             o = op_prepend_elem(OP_LINESEQ,
                     newOP(OP_ENTER, (o->op_flags & OPf_WANT)), o);
             OpTYPE_set(o, OP_LEAVE);
@@ -8984,6 +8987,11 @@ Perl_newCONDOP(pTHX_ I32 flags, OP *first, OP *trueop, OP *falseop)
 
     PERL_ARGS_ASSERT_NEWCONDOP;
 
+    if (OP_TYPE_IS(falseop, OP_STUB)) {
+        op_clear(falseop);
+        falseop = NULL;
+    }
+
     if (!falseop)
         return newLOGOP(OP_AND, 0, first, trueop);
     if (!trueop)

[iabyn]

On Wed, Feb 28, 2024 at 01:41:17PM -0800, Richard Leach wrote: The following experiment essentially results in the second example generating the first optree, not the second. Not 100% sure if that's the best approach, and even if it is, the patch is incomplete and needs work. ``` diff --git a/op.c b/op.c index 81073cc737..f63c0ac0eb 100644 --- a/op.c +++ b/op.c @@ -4487,6 +4487,9 @@ Perl_op_scope(pTHX_ OP *o) { if (o) { if (o->op_flags & OPf_PARENS || PERLDB_NOOPT || TAINTING_get) { + if (OP_TYPE_IS(o, OP_STUB)) + return o; +

I would be wary of skipping wrapping an OP_STUB in ENTER/LEAVE. It might be safe, or even preferable, but will have a wider impact than just skipping 'else'. Other code, both in core or on CPAN (e.g. code that uses B) may be relying on it to detect empty blocks and the like. Perhaps for now, Perl_newCONDOP() should check for *either* of a stub or enter/stub/leave? Then later, do the op_scope() thing as a separate optimisation? But apart from that, I like the idea of optimising away the 'else'. We should do a similar thing for an empty 'if' block, e.g. if ($cond) { # nothing needs doing } else { .... } Also, having just looked, I notice that an empty 'if' block is in fact only a stub without enter/leave, unlike an empty 'else' block.

…

-- I took leave and went to hear Mrs Turner's daughter play on the harpsicon, but Lord, it was enough to make any man sick to hear her; yet I was forced to commend her highly. -- The Diary of Samuel Pepys, 1 May 1663

[richardleach]

Thanks for those points, @iabyn.

I'd noticed last night that all tests pass on a debugging blead with the ENTER/LEAVE wrapping of STUB skipped like this:

@@ -4487,6 +4487,9 @@ Perl_op_scope(pTHX_ OP *o)
 {
     if (o) {
         if (o->op_flags & OPf_PARENS || PERLDB_NOOPT || TAINTING_get) {
+            if (OP_TYPE_IS(o, OP_STUB) && (o->op_flags & OPf_PARENS))
+                return o;
+
             o = op_prepend_elem(OP_LINESEQ,
                     newOP(OP_ENTER, (o->op_flags & OPf_WANT)), o);
             OpTYPE_set(o, OP_LEAVE);

However, as you mention, any impact on code outside of core is unknown at this point.

I need to spend time better understanding the minutiae of Perl_newCONDOP and how the op_next linking of its kids/parent occurs. Code like the following (from t/comp/retainedlines.t) breaks with the first attempt at optimisation, due to incorrect op_next threading beween the and and ternary.

    my %seen; my @keys = map { $_->[0] }
               sort { $b->[1] <=> $a->[1] }
               map { (!$seen{$_} and /eval (\d+)/) ? [ $_, $1 ] : ()  }
               keys %::;

Will look at the empty if block case as well.

[iabyn]

On Thu, Feb 29, 2024 at 03:25:28PM -0800, Richard Leach wrote: I need to spend time better understanding the minutiae of `Perl_newCONDOP` and how the `op_next` linking of its kids/parent occurs.

The section of code comments near the top of op.c may be of some help here, starting with "The execution order pointers (op_next) are generated ..."

…

-- Modern art: "That's easy, I could have done that!" "Ah, but you didn't!"

[richardleach]

Picked this up again. Can remove the else block and pass all existing tests. Need to fix up Deparse though, then move on to the empty if block case.

[richardleach]

Question: what should the behaviour be if both if and else branches are empty?

Codewise, it might be easiest to just assume that if the if block is empty, the else block isn't.

[richardleach]

PR for the else blocks: #22745

op.c changes for the if blocks also seem straightforward, but there's considerable breakage in assorted parts of Deparse.t to look at.