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:                }

[leonerd]

Rather than two nested ifs with no extra code, this might be neater and avoid a level of indent as if (!sib2 && (PL_op_args[sib1->op_type] & OA_RETSCALAR))

[leonerd]

Don't forget that under perl -d, these are OP_DBSTATE. I wrote a handy helper macro for such; OP_TYPE_IS_COP_NN.

if (OP_TYPE_IS_COP_NN(prev_kid) && OP_TYPE_IS_COP_NN(sib))

[leonerd]

The addition of this single brace that aligns with the one on the line above suggests something odd has happened with indentation here.

[leonerd]

I wonder if this should be neatened by adding a similar OP_TYPE_WAS_COP_NN() macro.

[richardleach]

Yeah, I could see that being useful.

Although since there's an OP_TYPE_IS_OR_WAS, does that mean we should also have an OP_TYPE_IS_OR_WAS_COP_NN? (Serious question, honest. 😝 )

[jkeenan]

@richardleach is this p.r. something we're going to (or should) pursue in the current dev cycle?

[richardleach]

is this p.r. something we're going to (or should) pursue in the current dev cycle?

No. I still want to dig around it before deciding what to do with it, but it definitely shouldn't be merged this close to a stable release. Adding the defer-next-dev label.