Remove defunct OPs in Perl_scalar/Perl_scalarvoid

[richardleach]

my $x = (1,2,3); produces the following OP tree in blead:

2     <;> nextstate(main 1 -e:1) v:{ ->3
6     <1> padsv_store[$x:1,2] vKS/LVINTRO ->7
5        <@> list sKP ->6
3           <0> pushmark v ->4
-           <0> ex-const v ->-
-           <0> ex-const v ->4
4           <$> const(IV 3) s ->5
-        <0> ex-padsv sRM*/LVINTRO ->6

This is functionally equivalent to my $x = 3;:

2     <;> nextstate(main 1 -e:1) v:{ ->3
4     <1> padsv_store[$x:1,2] vKS/LVINTRO ->5
3        <$> const(IV 3) s ->4
-        <0> ex-padsv sRM*/LVINTRO ->4

Construction of the first tree typically generates "Useless use of X in scalar context" warnings, but special cases such as the constants 0 and 1 are excluded from these warnings.

This commit modifies the functions responsible for assigning scalar or void context to OPs to remove:

• OP_NULL nodes with no kids and a following sibling.
• OP_LIST nodes with only a single-scalar-pushing kid OP.

This transforms the first OP tree above into the second.

Besides having a "cleaner-looking" optree that's easier to follow when debuggging Perl code or porting, there are other practical benefits:

• If the op_next chain hasn't been built, LINKLIST won't have to traverse these OP nodes and link them in. Subsequent compiler steps then won't re-traverse the same nodes to optimize them out of the op_next chain.
• Anything traversing - or cloning - the full optree has fewer defunct OP nodes to visit.
• OP slabs may contain a higher proportion of live OPs, reducing TLB pressure (on systems or workloads where that matters).

Note: this PR replaces #23523. It was easier to implement in op.c than I'd originally anticipated and, as mentioned above, doing so reduces the number of times such defunct nodes have to be processed during compilation.

---

• This set of changes requires a perldelta entry and one is included.

[tonycoz]

The change looks reasonable, though doing a coverage test they aren't hit much, the first part:

  3307622: 2074:                    if (OP_TYPE_IS(o, OP_LIST) && !op_parent(o)) {
        -: 2075:                        /* Is the list now just an obvious scalar pushop?
        -: 2076:                         *     <@> list sKP ->6
        -: 2077:                         *         <0> pushmark v ->4
        -: 2078:                         *         <$> const(IV 3) s ->5
        -: 2079:                         */
      110: 2080:                        OP* first = cLISTOPo->op_first;
        -: 2081:                        assert(OP_TYPE_IS(first, OP_PUSHMARK));
     110*: 2082:                        OP* sib1 = OpSIBLING(first);
        -: 2083:                        assert(sib1);
      110: 2084:                        OP* sib2 = OpSIBLING(sib1);
      110: 2085:                        if (!sib2) {
       27: 2086:                            if (
       27: 2087:                                PL_opargs[sib1->op_type] & OA_RETSCALAR
        -: 2088:                            ){
        -: 2089:                                assert(sib1->op_next == sib1);
        -: 2090:                                /* Yup. The PUSHMARK and LIST are redundant.
        -: 2091:                                 * They can be stripped out. */
       27: 2092:                                op_sibling_splice(o,first,1,NULL);
       27: 2093:                                op_free(o);
       27: 2094:                                return sib1;
        -: 2095:                            }
        -: 2096:                        }
        -: 2097:                    }

The second part:

     1853: 2113:                        if (kid->op_next == kid || kid->op_next == sib) {
     1853: 2114:                            if (prev_kid->op_next == kid)
     1558: 2115:                                prev_kid->op_next = kid->op_next;
        -: 2116:
     1853: 2117:                            prev_kid->op_sibparent = kid->op_sibparent;
     1853: 2118:                            op_free(kid); kid = NULL;
        -: 2119:
        -: 2120:                            /* A NEXTSTATE with no sibling OPs is redundant
        -: 2121:                             * if another NEXTSTATE follows it. Null it out
        -: 2122:                             * rather than removing it, in case anything needs
        -: 2123:                             * to probe it for file/line/hints info. */
     1853: 2124:                            if (OP_TYPE_IS(prev_kid, OP_NEXTSTATE) && sib
     1549: 2125:                                && OP_TYPE_IS(sib, OP_NEXTSTATE)) {
        1: 2126:                                op_null(prev_kid);
        -: 2127:                            }
        -: 2128:                        }

[richardleach]

The change looks reasonable, though doing a coverage test they aren't hit much

Yeah, this is true. I'm not sure how much that will be inherently true and how much is because the test cases are well written - and some code in the wild might benefit more?

The addition to S_voidnonfinal is most exercised by the test harness:

 64525641: 2813:                    if (OP_TYPE_IS(kid, OP_NULL) &&
  6229022: 2814:                       !(kid->op_flags & OPf_KIDS) &&
        -: 2815:                       /* Perl_leaveeval needs an ex-nextstate for its
        -: 2816:                          feature state information */
    58006: 2817:                       kid->op_targ != OP_NEXTSTATE &&
    55045: 2818:                       kid->op_targ != OP_DBSTATE
        -: 2819:                    ){
        -: 2820:                        /* This kid is no longer needed. */
    55045: 2821:                        if (prev_kid) {
        -: 2822:                            assert(prev_kid->op_next != kid);
    55028: 2823:                            op_sibling_splice(o,prev_kid,1,NULL);
        -: 2824:                        } else {
        -: 2825:                            assert(op_parent(kid)->op_next != kid);
       17: 2826:                            op_sibling_splice(o,NULL,1,NULL);
        -: 2827:                        }
    55045: 2828:                        op_free(kid);
        -: 2829:                    }
        -: 2830:                }